fix(ci): resolve all zizmor findings and add zizmor pre-commit checks - #404
Conversation
gforsyth
added
improvement
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Left some suggestions for your consideration, but I'm also comfortable with this being merged as-is if you think they're not worth implementing.
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v6 | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 |
There was a problem hiding this comment.
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 | |
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 |
This is v6.0.2 (https://github.com/actions/checkout/releases/tag/v6.0.2.
I think specifying that full tag might be useful for renovate's update logic? I'm not sure, but recommend erring on the side of pinning to a specific semver where it's available.
If nothing else, I think using the full version should mean the diff for the next automatic update will change the comment too, so we'll be able to tell the difference visually between 6.0.2 -> 6.0.3 and 6.0.2 -> 6.1.0.
Would you consider that for all of these?
There was a problem hiding this comment.
Yeah, the tags vs. releases are weird -- I think our mutable refs are pointing at tags (one more reason not to use them) -- so it is 6.0.2 but it is also v6 (https://github.com/actions/checkout/releases/tag/v6) -- and I think they change what v6 refers to for point releases. I can update that.
There was a problem hiding this comment.
I agree with your read, that's exactly right.
v6 gets updated every time a new v6.* is cut.
| with: | ||
| build_type: pull-request | ||
| secrets: inherit | ||
| secrets: inherit # zizmor: ignore[secrets-inherit] |
There was a problem hiding this comment.
Instead of secrets: inherit, would you consider explicitly defining which secrets need to get passed through in workflow_call calls?
Here's an example of that: rapidsai/shared-workflows#489 (though here we don't need the name and value separated, can just pass through the value directly).
That's tighter than secrets: inherit because it means that new secrets that become available to the repo aren't immediately accessible in the calling workflow.
I also think it has the nice side benefit of making the configuration flow a bit more explicit.
There was a problem hiding this comment.
I agree it's better to explicilty define the secrets -- and because it's currently all implicit it's hard to trace out which secrets are needed.
I might defer this until we can create a general grammar for which shared workflows make use of which secrets and then take a pass to tighten up the secret passing.
There was a problem hiding this comment.
I have more thoughts on this after some spelunking but I think it should get written up somewhere else.
I'll take a pass on this PR, though, and tighten up the secrets usage where possible (it's largely not possible at the moment)
There was a problem hiding this comment.
xref rapidsai/build-infra#358
There was a problem hiding this comment.
Latest changes + that issue look good, thank you for that!
|
Pulled the latest changes into this PR This also reruns CI. Hoping this clears out the previous |
|
/merge |
3 participants
Similar to upstream changes in
shared-workflows, this PR cleans up and annotates all of the workflows and adds thezizmorlinter to make sure changes are checked.Part of rapidsai/build-planning#275