Conversation
janbrasna
left a comment
janbrasna
left a comment
There was a problem hiding this comment.
Noticed one action missing from the update, and also general PR/branch trigger observation:
| - test | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
One forgotten instance also down here for consistency:
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 |
There was a problem hiding this comment.
BTW hard-pinning actions asks for setting up dependabot schedule, to keep updating these over time, to not get stuck on an outdated or insecure version when overspecified like this. (Or does implicit dependabot, without ecosystem configs, only submitting bumps on vulnerabilities, also cover GHA versions if there's a sec bump necessary?)
There was a problem hiding this comment.
If I'm understanding the question correctly, dependabot can automatically update these when pinned to a hash, for example: mozilla/bigquery-etl#9069
There was a problem hiding this comment.
That repository has an explicit ecosystem configuration I was hinting at:
https://github.com/mozilla/bigquery-etl/blob/main/.github/dependabot.yml
There was a problem hiding this comment.
I see what you're saying. Dependabot security updates are enabled in the repo settings so I would expect it to be able to update GHA as well. However, automated PRs are currently disabled because this repo has been mostly unmaintained for 5 years
There was a problem hiding this comment.
I have never tried or relied on it myself before, so don't have experience to share unfortunately — but also hope like with any other ecosystem, if no schedule provided, the bumps will come "loudly" in case of a vulnerability fix, even for factory GHA actions.
Just left here this observation that hard pinning actions introduces new level of maintenance burden, hopefully these versions will be good for some time now;) Thanks for bumping these!
| docker run --rm --tty --env-file=.env ensemble-transposer npm test | ||
| deploy: | ||
| runs-on: ubuntu-latest | ||
| if: github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/') |
There was a problem hiding this comment.
This still runs on: push: * so it would tag any branch/PRs' refs as "latest" which is perhaps not the intention?
Wondering if the opposite, i.e. running on: push, pull_request to test/lint also PRs from forks/contributors would be helpful, and then keep the default branch +tag guardrail like this above ^^ in?
There was a problem hiding this comment.
(It also seems "test" is a required check to pass on a PR, but that's currently not being run for PRs coming from forks, FYI — so these can never pass not even triggering that pipeline, as seen on my PR's checks: #243)
There was a problem hiding this comment.
This was removed temporarily to test the workflow from this branch. I forgot about this PR so I didn't end up removing it.
The inability to run for forked PRs is something we're aware of and we have a solution for some other repos, but we default to not running for forks for security reasons. I can manually test your PR and bypass the check in this case
There was a problem hiding this comment.
Ah okay. That can be set up systemically https://docs.github.com/en/actions/how-tos/manage-workflow-runs/approve-runs-from-forks and allow manual approvals after inspection if need be.
If there's a way around the missing checks, no probs then of course! I always get caught by surprise by the mismatch of expected run names required to merge for environment protection, and their potential disconnection from the actual CI runs on the PRs;)
(I'm adding just a few JSON translations, so it should be both safe and uncontroversial, thanks!)
2 participants
https://mozilla-hub.atlassian.net/browse/DENG-10780