ci: apply github workflow best practice (zizmor/codeql/asf-allowlist-check)#4216
ci: apply github workflow best practice (zizmor/codeql/asf-allowlist-check)#4216kevinjqliu wants to merge 5 commits intoapache:mainfrom
Conversation
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
@adutra could you take a look?
| persist-credentials: false | ||
| # Intentionally unpinned to always use the latest allowlist from the ASF. | ||
| - uses: apache/infrastructure-actions/allowlist-check@main # zizmor: ignore[unpinned-uses] | ||
| - uses: apache/infrastructure-actions/allowlist-check@4e9c961f587f72b170874b6f5cd4ac15f7f26eb8 # main |
There was a problem hiding this comment.
made improvements to apache/infrastructure-actions/allowlist-check so that we no longer need to pin to @main
There was a problem hiding this comment.
Do you know how Renovate will handle this one?
There was a problem hiding this comment.
i dont, i havent used renovate before, but this github action shouldnt change much https://github.com/apache/infrastructure-actions/tree/main/allowlist-check
There was a problem hiding this comment.
Hm, the question is: who or what cares about updating the action? I suspect, Git tags/releases would be difficult, because that would either tie the apache/infrastructure-actions repo to only one action or all actions happen on the same release schedule?
| name: Run zizmor 🌈 | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| security-events: write | ||
| permissions: {} | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 | ||
| - name: Checkout repository | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| persist-credentials: false | ||
| - name: Run Zizmor (SARIF upload) | ||
| uses: zizmorcore/zizmor-action@71321a20a9ded102f6e9ce5718a2fcec2c4f70d8 # v0.5.2 | ||
| with: | ||
| min-severity: medium | ||
| min-confidence: medium | ||
| - name: Run Zizmor (PR annotations) | ||
|
|
||
| - name: Run zizmor 🌈 | ||
| uses: zizmorcore/zizmor-action@71321a20a9ded102f6e9ce5718a2fcec2c4f70d8 # v0.5.2 | ||
| with: |
There was a problem hiding this comment.
making this change so that zizmor ci check will fail when encountering an issue. otherwise, it will pass and add to the "Security" tab in github
There was a problem hiding this comment.
But that's why I included two steps: one with advanced security on, the other with it off. Wasn't that working as expected?
There was a problem hiding this comment.
oh good point, i missed that.
it might not be necessary to run both, the one with advanced-security: false will fail CI. then there's no need to upload SARIF. wdyt?
There was a problem hiding this comment.
Well, I'd rather have advanced security on, tbh, since that's the recommended way to use the action.
If we are concerned by a PR that could succeed while it contains security warnings, the documentation page says:
Continue to use zizmor-action with advanced-security: true, but configure a ruleset to prevent PRs from merging until all code scanning alerts are resolved. This is the recommended approach, but you must configure it manually — zizmor-action cannot do it for you.
I cannot create rulesets though, but it seems to me it's just a matter of having someone create a ruleset like this one.
Until we have this sorted out, I figured that having two steps (one with advanced security on, the other without) would bring the best of both worlds.
There was a problem hiding this comment.
I reverted the changes to zizmor in this PR.
In the iceberg repo, the behavior is to block in CI which is the behavior when advanced-security: false
The current implementation will both block in CI (the PR annotations part) and report to the Security tab (SARIF upload). My point was that we dont need to do both. If its already blocked in CI, there's not really a need to also report it to the Security tab
kevinjqliu
changed the title
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Thanks @kevinjqliu ! We will need a rebase.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
kevinjqliu
force-pushed
the
kevinjqliu/zizmor
branch
from
49d53d3 to
5b04513
Compare
|
rebased, thanks! |
| persist-credentials: false | ||
| # Intentionally unpinned to always use the latest allowlist from the ASF. | ||
| - uses: apache/infrastructure-actions/allowlist-check@main # zizmor: ignore[unpinned-uses] | ||
| - uses: apache/infrastructure-actions/allowlist-check@4e9c961f587f72b170874b6f5cd4ac15f7f26eb8 # main |
There was a problem hiding this comment.
Do you know how Renovate will handle this one?
|
|
||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4 | ||
| uses: github/codeql-action/init@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4.35.2 |
There was a problem hiding this comment.
There was a problem hiding this comment.
Not sure this comment-only change is worth it. Renovate will overwrite it anyways.
|
kept the changes minimal, removed the zizmor changes. ptal! |
4 participants
Follow up to #4097
Based on a few more recent improvements made in the apache/iceberg repo