Check the repository owner in CI tasks #1280
Conversation
| if: runner.os == 'Linux' | ||
| if: runner.os == 'Linux' && github.repository_owner == 'GaloisInc' | ||
| uses: cachix/install-nix-action@v16 | ||
| with: | ||
| nix_path: nixpkgs=channel:21.11 |
There was a problem hiding this comment.
I'm not entirely sure that it's correct to guard this entire step behind github.repository_owner == 'GaloisInc', as the remainder of the workflow depends on Nix being installed in order to work correctly. The only reason that we require a secret value in the first place is to fill out access-tokens, which is solely for the sake of avoiding rate limiting issues. As such, it is likely more correct just to leave out the extra_nix_config portion of this step if a fork is being used...
...but then again, even that might not be necessary. If I'm reading the docs correctly, then secrets.GITHUB_TOKEN is always defined, so I'm inclined to think that this part simply won't be an issue for forks. As such, I'm inclined not to change anything about the install-nix-action steps. (Please speak up if I've overlooked something, however.)
There was a problem hiding this comment.
I was wondering about that. And I was wondering why it needed a token to get at Nix.
I don't know enough about the internal workings to second-guess you :-) so I'll remove these.
There was a problem hiding this comment.
oof.. it's hard to find where this conversation happened because you force pushed. One of the reasons I dislike force pushes except in dire cases.
Ryan's info is correct though: nothing really to do with nix, we are just avoiding github's rather severe rate limiting using a token that is provided for this particular workflow run. Please feel free to add a comment to this effect to impede future confusion.
There was a problem hiding this comment.
Yeah, I thought it was going to be force-push a minor adjustment then merge, but you got to it before I came back 😸. (Which is just as well because this way there doesn't need to be a whole additional pull request...)
sauclovian-g
force-pushed
the
tidy-ci-in-forks
branch
from
b93d16e to
a8dfa60
Compare
| build-push-image: | ||
| runs-on: ubuntu-22.04 | ||
| needs: [config] | ||
| if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || needs.config.outputs.release == 'true' | ||
| if: (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || needs.config.outputs.release == 'true') && github.repository_owner == 'GaloisInc' |
There was a problem hiding this comment.
The build-push-image job in crux-mir-build.yml should be guarded in a similar way.
There was a problem hiding this comment.
hmm, not sure how I missed that. I've added it
This will prevent them from running and failing in forked repos.
sauclovian-g
force-pushed
the
tidy-ci-in-forks
branch
from
a8dfa60 to
032894a
Compare
|
Looking in crux-mir-build.yml, I notice we have and then immediately after it, which is one of the ones I updated. For tidiness they should probably have the same check (since if we don't create the binary artefact, signing it will certainly fail)... which one do we prefer? I think the chances of ever renaming the crucible repo are 0 and that's probably the only functional difference. |
|
I'm fine with checking for |
Also use the same conditional for building, signing, and uploading the same artefacts.
Compare GaloisInc/cryptol#1792.