Copilot commented on code in PR #204:
URL:
https://github.com/apache/cloudstack-cloudmonkey/pull/204#discussion_r2789592047
##########
.github/workflows/build-pr-cmk.yml:
##########
@@ -36,10 +36,9 @@ jobs:
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
Review 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.
##########
.github/workflows/build-pr-cmk.yml:
##########
@@ -68,53 +67,3 @@ jobs:
run: |
echo "outcome=${{ steps.build.outcome }}" >> $GITHUB_OUTPUT
echo "artifact_url=${{ steps.upload_artifact.outputs.artifact-url
}}" >> $GITHUB_OUTPUT
Review 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'`).
##########
.github/workflows/comment-pr-build.yml:
##########
@@ -0,0 +1,153 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Comment on PR build results
+
+on:
+ workflow_run:
+ workflows: ["Build cmk binaries on PR"]
+ types:
+ - completed
+
+permissions:
+ contents: read
+ issues: write
+ pull-requests: write
+
Review 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).
##########
.github/workflows/build-pr-cmk.yml:
##########
@@ -36,10 +36,9 @@ jobs:
outcome: ${{ steps.meta.outputs.outcome }}
artifact_url: ${{ steps.meta.outputs.artifact_url }}
steps:
Review 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.
##########
.github/workflows/comment-pr-build.yml:
##########
@@ -0,0 +1,153 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Comment on PR build results
+
+on:
+ workflow_run:
+ workflows: ["Build cmk binaries on PR"]
+ types:
+ - completed
+
+permissions:
+ contents: read
+ issues: write
+ pull-requests: write
+
+jobs:
+ comment:
+ runs-on: ubuntu-24.04
+ if: >
+ github.event.workflow_run.event == 'pull_request'
+ steps:
+ - name: Download artifact metadata
+ uses: actions/github-script@v7
+ id: artifact-metadata
+ with:
+ script: |
+ const artifacts = await
github.rest.actions.listWorkflowRunArtifacts({
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ run_id: context.payload.workflow_run.id,
+ });
+
+ const prArtifact = artifacts.data.artifacts.find(a =>
a.name.startsWith('cmk-binaries.pr'));
+
+ if (prArtifact) {
+ const prNumber = prArtifact.name.match(/pr(\d+)/)?.[1];
+ return {
+ artifact_url: prArtifact.archive_download_url,
+ pr_number: prNumber,
+ conclusion: context.payload.workflow_run.conclusion
+ };
+ }
+
+ return {
+ pr_number: null,
+ conclusion: context.payload.workflow_run.conclusion
+ };
+
+ - name: Get PR number from workflow run
+ id: get-pr
+ uses: actions/github-script@v7
+ with:
+ script: |
+ 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;
+
Review 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.
```suggestion
// 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 }}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
