Skip to content

CI: Query base.ref instead of sha for milestone VERSION resolution#7463

Open
ArthPatel1502 wants to merge 1 commit into
OSGeo:mainfrom
ArthPatel1502:fix/assign-milestone-target-7374
Open

CI: Query base.ref instead of sha for milestone VERSION resolution#7463
ArthPatel1502 wants to merge 1 commit into
OSGeo:mainfrom
ArthPatel1502:fix/assign-milestone-target-7374

Conversation

@ArthPatel1502
Copy link
Copy Markdown
Contributor

Closes #7374. Updates the milestones workflow to look up the version file using the target PR base branch ref instead of the commit SHA.

Comment thread .github/workflows/milestones.yml Outdated
-H "Accept: application/vnd.github.raw" \
-H "X-GitHub-Api-Version: 2022-11-28" \
"/repos/{owner}/{repo}/contents/include/VERSION?ref=${{ github.sha }}"
"/repos/{owner}/{repo}/contents/include/VERSION?ref=${{ github.event.pull_request.base.ref }}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about merge_commit_sha in https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=closed#pull_request

In https://docs.github.com/en/rest/pulls/pulls?apiVersion=2026-03-10#get-a-pull-request, it shows that if the PR is merged (which is what is checked on line 13, then it is a sha (so less user-controllable).

Suggested change
"/repos/{owner}/{repo}/contents/include/VERSION?ref=${{ github.event.pull_request.base.ref }}"
"/repos/{owner}/{repo}/contents/include/VERSION?ref=${{ github.event.pull_request. merge_commit_sha }}"

It might still be needed to change into an env var to not evaluate the template directly for zizmor checks to be happy (which is a good thing).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great catch and the security tip, @echoix! That makes total sense. I've updated the workflow to use merge_commit_sha and safely mapped it to an environment variable to keep zizmor happy. Appreciate the feedback!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When or where did you update it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, @echoix ! I realized I hadn't actually committed the new changes over my previous commit on this branch. I have just pushed the updated version using merge_commit_sha passed via an environment variable. Thanks for your patience!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you need to push the commit too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have successfully updated the branch, @echoix! The workflow now uses merge_commit_sha mapped safely to an environment variable (MERGE_SHA) to satisfy the security requirements. The changes are ready for your review!

@github-actions github-actions Bot added the CI Continuous integration label Jun 1, 2026
@ArthPatel1502 ArthPatel1502 force-pushed the fix/assign-milestone-target-7374 branch from 4e66ed8 to 5736524 Compare June 5, 2026 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Revise Assign Milestone Workflow

2 participants