ci: grant actions:write to PR review workflow - #14004
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| checks: write # (Optional) Show review progress as a check run | ||
| id-token: write # Required for OIDC authentication to AWS Secrets Manager | ||
| actions: read # Download artifacts from trigger workflow | ||
| actions: write # Download artifacts from trigger workflow; cache delete for review-lock release (required since docker-agent-action v2.0.3) |
There was a problem hiding this comment.
Looks good but reusable workflows should never define permissions: https://github.com/docker/docker-agent-action/blob/5396207026ca7d22d0a06a706260b7eb671c9346/.github/workflows/review-pr.yml#L86-L91
It should be only the caller to define them. More info:
There was a problem hiding this comment.
reusable workflows should never define permissions
I think reusable workflows should define the minimum permissions they need, following least-privilege.
There was a problem hiding this comment.
It also causes consumers to fail fast/loud if they're missing required permissions. We've run into silent/annoying errors in the past.
There was a problem hiding this comment.
following least-privilege.
It doesn't work like that unfortunately: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations#supported-keywords-for-jobs-that-call-a-reusable-workflow
... permissions passed from the caller workflow can be only downgraded (not elevated) by the called workflow.
For example in our reusable workflow we don't need packages: write for registries other than GHCR and if we define packages: read it cannot be elevated:
The nested job 'build' is requesting 'packages: write', but is only allowed 'packages: read'.
Not sure about docker/docker-agent-action pr-review workflow need all these write access?
So defining elevated permissions even if not needed feels over-scope
There was a problem hiding this comment.
Not sure about docker/docker-agent-action pr-review workflow need all these write access?
Yeah, we got to the permissions we have now incrementally as we ran into failures by consumers (I have like half a dozen of my own PRs just bumping individual permissions, similar to this PR).
It doesn't work like that unfortunately: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations#supported-keywords-for-jobs-that-call-a-reusable-workflow
That's interesting, I understood that doc as implying reusable workflows SHOULD define their own permissions but only to downgrade, rather than elevate. Thank you for sharing that!
Regardless, all of this is moot once our AI reviewer is moved off of GHA and into our own infra, which is currently in progress.
docker-agent-action v2.0.3 raised the review job's required permission from actions:read to actions:write (cache delete for review-lock release, feedback artifact management). GitHub refuses to start a reusable workflow requesting more permissions than the caller grants, so every PR Review run since the v2.0.3 bump ended in startup_failure and docker-agent stopped launching automatically. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
b27cb5a to
22120ea
Compare
3 participants
What I did
docker-agent-action v2.0.3 raised the review job's required permission from actions:read to actions:write (cache delete for review-lock release, feedback artifact management). GitHub refuses to start a reusable workflow requesting more permissions than the caller grants, so every PR Review run since the v2.0.3 bump ended in startup_failure and docker-agent stopped launching automatically.
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did
