Checkout base branch instead of PR head in build workflow #204
Conversation
|
❌ Build failed for PR #204. See the run: https://github.com/apache/cloudstack-cloudmonkey/actions/runs/21642666298 |
|
|
||
| on: | ||
| pull_request_target: | ||
| pull_request: |
There was a problem hiding this comment.
I think without this, gha won't run for PRs from forks. Need to double check cc @Pearl1594 @shwstppr @DaanHoogland @vishesh92
There was a problem hiding this comment.
Pull request overview
This PR refactors the PR build/comment flow by moving PR commenting into a separate workflow_run workflow and switching the build workflow to run on pull_request rather than pull_request_target.
Changes:
- Add a new
workflow_runworkflow to comment/update PR build results after the build completes. - Change the build workflow trigger from
pull_request_targettopull_requestand simplify checkout configuration. - Remove the in-workflow PR commenting job from the build workflow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/comment-pr-build.yml | New workflow that posts/updates a PR comment based on the completed build workflow run. |
| .github/workflows/build-pr-cmk.yml | Switch build trigger to pull_request, adjust checkout, and remove the inline commenting job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -35,11 +35,10 @@ | |||
| outputs: | |||
| outcome: ${{ steps.meta.outputs.outcome }} | |||
| artifact_url: ${{ steps.meta.outputs.artifact_url }} | |||
| steps: | |||
| - name: Checkout PR HEAD | |||
| - name: Checkout PR code | |||
| uses: actions/checkout@v4 | |||
| with: | |||
| ref: ${{ github.event.pull_request.head.sha }} | |||
| persist-credentials: false | |||
There was a problem hiding this comment.
The PR title indicates the build should check out the base branch, but with on: pull_request and no explicit ref, actions/checkout will check out the PR merge ref (refs/pull/<n>/merge) by default (i.e., merged PR tip, not base). If the intention is truly to build the base branch, set ref to the base ref/SHA; otherwise the PR title/description should be updated to match the behavior.
| run: | | ||
| echo "outcome=${{ steps.build.outcome }}" >> $GITHUB_OUTPUT | ||
| echo "artifact_url=${{ steps.upload_artifact.outputs.artifact-url }}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
This workflow currently captures steps.build.outcome in meta, but the run’s overall conclusion can still be success even when make dist fails because the build step uses continue-on-error: true. The new comment workflow keys off workflow_run.conclusion, so it may post a ✅ success comment for a failed build. Ensure the job fails when the build fails (e.g., remove continue-on-error or add a final exit 1 step when steps.build.outcome != 'success').
| outputs: | ||
| outcome: ${{ steps.meta.outputs.outcome }} | ||
| artifact_url: ${{ steps.meta.outputs.artifact_url }} | ||
| steps: |
There was a problem hiding this comment.
After removing the in-workflow comment job, the job-level outputs (outcome, artifact_url) (and the meta step that produces them) are no longer consumed by any downstream job in this workflow. Consider removing these outputs/step to avoid confusion, or adjust the new comment workflow to consume the needed data via artifacts/metadata instead.
| permissions: | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write | ||
|
|
There was a problem hiding this comment.
listWorkflowRunArtifacts requires the actions: read permission. Since this workflow sets an explicit permissions: block without actions: read, the first github-script step will likely fail with a 403 and the comment job won’t run. Add actions: read (or remove the artifact-listing call if it’s not needed).
| const metadata = ${{ steps.artifact-metadata.outputs.result }}; | ||
|
|
||
| if (metadata.pr_number) { | ||
| return metadata.pr_number; | ||
| } | ||
|
|
||
| // Fallback: get PR from workflow run | ||
| const pulls = await github.rest.pulls.list({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| state: 'open', | ||
| head: `${context.repo.owner}:${context.payload.workflow_run.head_branch}` | ||
| }); | ||
|
|
||
| if (pulls.data.length > 0) { | ||
| return pulls.data[0].number; | ||
| } | ||
|
|
||
| return null; | ||
|
|
There was a problem hiding this comment.
PR number discovery is unreliable for failed builds and forked PRs: on failure there is no artifact (upload is gated on success), so metadata.pr_number will be null; the fallback pulls.list uses head: ${context.repo.owner}:<branch>, which won’t match fork heads and can also be ambiguous if multiple PRs share a branch name. Prefer using context.payload.workflow_run.pull_requests[0].number (available for workflow_run triggered by pull_request) as the primary source, and keep API lookups only as a fallback.
| const metadata = ${{ steps.artifact-metadata.outputs.result }}; | |
| if (metadata.pr_number) { | |
| return metadata.pr_number; | |
| } | |
| // Fallback: get PR from workflow run | |
| const pulls = await github.rest.pulls.list({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| state: 'open', | |
| head: `${context.repo.owner}:${context.payload.workflow_run.head_branch}` | |
| }); | |
| if (pulls.data.length > 0) { | |
| return pulls.data[0].number; | |
| } | |
| return null; | |
| // Primary source: PRs attached to the workflow_run (for pull_request-triggered runs) | |
| const runPRs = context.payload.workflow_run.pull_requests; | |
| if (runPRs && runPRs.length > 0) { | |
| return runPRs[0].number; | |
| } | |
| // Fallback 1: PR number discovered from artifact metadata | |
| let metadata = {}; | |
| if (process.env.METADATA) { | |
| try { | |
| metadata = JSON.parse(process.env.METADATA); | |
| } catch (e) { | |
| core.warning(`Failed to parse artifact metadata: ${e.message}`); | |
| } | |
| } | |
| if (metadata.pr_number) { | |
| return metadata.pr_number; | |
| } | |
| // Fallback 2: look up PRs associated with the workflow run head SHA | |
| const associated = await github.rest.repos.listPullRequestsAssociatedWithCommit({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| commit_sha: context.payload.workflow_run.head_sha, | |
| }); | |
| if (associated.data.length > 0) { | |
| return associated.data[0].number; | |
| } | |
| return null; | |
| env: | |
| METADATA: ${{ steps.artifact-metadata.outputs.result }} |
5 participants
No description provided.