Support CI PR contributions#45
Merged
Merged
Conversation
Reviewer's GuideRefactors the main container workflow to support being called from other workflows with explicit PR context, introduces ref/tag resolution logic, and adds a new pull_request_target/issue_comment workflow to safely run container builds for forked PRs with maintainer-controlled triggering. Sequence diagram for maintainer-approved CI on forked pull requestssequenceDiagram
actor Contributor
actor Maintainer
participant GitHub
participant container_pr_notify as container-pr.yml_notify
participant container_pr_resolve as container-pr.yml_resolve
participant container_pr_build as container-pr.yml_build
participant container_workflow as container.yml_build
participant TestingFarm
participant GHCR
Contributor->>GitHub: Open fork pull_request
GitHub-->>container_pr_notify: pull_request_target opened
GitHub-->>container_pr_resolve: pull_request_target opened
alt Untrusted contributor (not OWNER/MEMBER/COLLABORATOR)
container_pr_notify->>GitHub: Post comment (CI requires maintainer /test)
note over container_pr_resolve: resolve job condition not satisfied
end
Maintainer->>GitHub: Add issue_comment "/test"
GitHub-->>container_pr_resolve: issue_comment created
alt Trusted maintainer comment
container_pr_resolve->>GitHub: Fetch PR info via gh api
container_pr_resolve->>container_pr_resolve: Determine pr_number and git_sha
container_pr_resolve-->>container_pr_build: Expose outputs git_sha, pr_number
container_pr_build-->>container_workflow: workflow_call container.yml
activate container_workflow
container_workflow->>container_workflow: Resolve refs (prefer inputs git_sha, pr_number)
container_workflow->>container_workflow: Compute image tag (pr-<number>-<version>)
container_workflow->>TestingFarm: Schedule build
container_workflow->>GHCR: Use GHCR_TOKEN for auth
deactivate container_workflow
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
/testtrigger condition will match any comment starting with/test(e.g./testfoo); if you intend to only allow a specific command, consider tightening the check (e.g. exact match or using a regex for/test(\s|$)after trimming). - The OWNER/MEMBER/COLLABORATOR checks are duplicated in both the
pull_request_targetandissue_commentconditions; consider extracting this into a single reusable expression viaenvor a job-level boolean to reduce maintenance overhead and the risk of divergence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `/test` trigger condition will match any comment starting with `/test` (e.g. `/testfoo`); if you intend to only allow a specific command, consider tightening the check (e.g. exact match or using a regex for `/test(\s|$)` after trimming).
- The OWNER/MEMBER/COLLABORATOR checks are duplicated in both the `pull_request_target` and `issue_comment` conditions; consider extracting this into a single reusable expression via `env` or a job-level boolean to reduce maintenance overhead and the risk of divergence.
## Individual Comments
### Comment 1
<location path=".github/workflows/container.yml" line_range="11-14" />
<code_context>
schedule:
- cron: '0 4 * * 1'
workflow_dispatch:
+ workflow_call:
+ inputs:
+ git_sha:
+ required: true
+ type: string
+ pr_number:
</code_context>
<issue_to_address>
**issue (bug_risk):** Mixing `workflow_call` with `push/schedule/workflow_dispatch` can break because `inputs.*` is not available for non-reusable invocations.
Because this file now mixes normal triggers with `workflow_call`, any unconditional use of `inputs.*` (e.g. `inputs.pr_number` / `inputs.git_sha` in the `Resolve refs` step) will fail validation for `push`/`schedule`/`workflow_dispatch` runs, where the `inputs` context doesn’t exist.
To keep all triggers working, either:
- Move the reusable workflow (`workflow_call`) into its own file, or
- Guard `inputs.*` behind `if: ${{ github.event_name == 'workflow_call' }}` and use `github.sha` / event data for other triggers, or
- Restrict `inputs.*` to a job that only runs for `workflow_call`.
As written, non-`workflow_call` invocations are likely to break.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
thrix
force-pushed
the
accept-pr-contributions
branch
2 times, most recently
from
ccc3de8 to
52e65b2
Compare
Owner
Author
|
@sourcery-ai please review |
|
Sure! I'm generating a new review now. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
/testtrigger incontainer-pr.ymluses an exact string comparison (github.event.comment.body == '/test'), which is quite brittle; consider allowing leading/trailing whitespace or commands like/test something(e.g., viastartsWithor simple shell parsing) to make maintainer usage less error-prone. - In the
notifyjob message you describe CI being triggered by a maintainer comment/test, but there is no mention of restrictions (e.g., only OWNER/MEMBER/COLLABORATOR) or the fact that same‑repo PRs are handled differently; consider updating the wording to more precisely reflect when and how contributors can expect tests to run.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `/test` trigger in `container-pr.yml` uses an exact string comparison (`github.event.comment.body == '/test'`), which is quite brittle; consider allowing leading/trailing whitespace or commands like `/test something` (e.g., via `startsWith` or simple shell parsing) to make maintainer usage less error-prone.
- In the `notify` job message you describe CI being triggered by a maintainer comment `/test`, but there is no mention of restrictions (e.g., only OWNER/MEMBER/COLLABORATOR) or the fact that same‑repo PRs are handled differently; consider updating the wording to more precisely reflect when and how contributors can expect tests to run.
## Individual Comments
### Comment 1
<location path=".github/workflows/container-pr.yml" line_range="12-17" />
<code_context>
+ issue_comment:
+ types: [created]
+
+permissions: {}
+
+jobs:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider explicitly scoping `GITHUB_TOKEN` permissions to the minimal fields used by `gh`.
You already use `permissions: {}` at the workflow level and grant `pull-requests: write/read` at the job level, which is good. Since `gh` here only posts comments and reads PR metadata, you can likely narrow this to `permissions: { issues: write }` for the commenting job (it uses the issues comments API) and `pull-requests: read` only where PR data is needed. This keeps the token closer to least-privilege.
```suggestion
jobs:
notify:
runs-on: ubuntu-latest
permissions:
issues: write
pull-requests: read
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hey @thrix, I've posted a new review for you! |
thrix
force-pushed
the
accept-pr-contributions
branch
from
52e65b2 to
ba1dfcb
Compare
Add `container-pr.yml` workflow to handle fork PR contributions:
- Same-repo PRs trigger `container.yml` directly via `pull_request`
(secrets available, runs immediately)
- Fork PRs from trusted users (OWNER/MEMBER/COLLABORATOR) auto-trigger
via `pull_request_target` in `container-pr.yml`
- External fork PRs require a maintainer to comment `/test` to trigger
the build (`issue_comment` trigger)
- A comment is posted on newly opened external fork PRs informing the
contributor that CI requires maintainer approval
`container-pr.yml` resolves the PR head SHA and calls `container.yml`
as a reusable workflow via `workflow_call`, avoiding matrix duplication.
Security improvements:
- Pass all GitHub context values through `env:` instead of inline
`${{ }}` in `run:` blocks to prevent template injection
- Add explicit `permissions: {}` and least-privilege job permissions
Assisted-by: Claude Code
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
thrix
force-pushed
the
accept-pr-contributions
branch
from
ba1dfcb to
8636bcb
Compare
Owner
Author
|
@sourcery-ai please review |
|
Sure! I'm generating a new review now. |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The trusted roles check (
OWNER/MEMBER/COLLABORATOR) is duplicated in multipleifexpressions; consider extracting this into a single reusable expression (viaenvor a composite action) to avoid drift if the trusted set ever changes. - The
/testtrigger currently requires the comment to start exactly with/test; if you intend to support variations like leading whitespace or casing (/Test,/test foo), consider normalizing the body or using a more flexible pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The trusted roles check (`OWNER`/`MEMBER`/`COLLABORATOR`) is duplicated in multiple `if` expressions; consider extracting this into a single reusable expression (via `env` or a composite action) to avoid drift if the trusted set ever changes.
- The `/test` trigger currently requires the comment to start exactly with `/test`; if you intend to support variations like leading whitespace or casing (`/Test`, `/test foo`), consider normalizing the body or using a more flexible pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Hey @thrix, I've posted a new review for you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Gate container image CI for pull requests from forks, requiring maintainer approval while allowing trusted and same-repo contributions to run automatically.
New Features:
Enhancements: