Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GHA: Use SHA from PR HEAD commit for pkg name #1124

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

tleedjarv
Copy link
Contributor

I'm opening this PR for discussion (see #1123 (comment)) but I'm not sure it should be merged, as there are some caveats.

The CI script still builds the merge commit. It is possible to change this and build the PR HEAD, but I'm not sure it is a good idea. The point of CI is to run the build tests before merging and the PR HEAD might be way off from the actual merge result (if it hasn't been rebased recently, for example).

With CI building the merge commit (which I think is a good idea), is it actually a good idea to name the build artifacts with the SHA from PR HEAD? If the PR was not rebased recently then the build artifacts are not based off that HEAD, strictly speaking.

(Note that all of this discussion only matters for open PRs; CI builds on merges, direct pushes and tagging are unaffected.)

By default GHA provides SHA of the merge commit even for unmerged PRs.
This made matching build artifacts to the actual commit impossible.
@gdt
Copy link
Collaborator

gdt commented Feb 19, 2025

I see your point about the merge commit being necessary. As someone who believes in vigorous rebasing, I wasn't thinking about the PR commit and the resulting merge having different trees. And, I also see your point about not labeling an artifact to be about the tip commit on a PR when it is the merge that is built.

The problem is that this makes it hard to tie the artifact back to the sources it came from.

Perhaps put "unmerged-pr-" or something in the artifact name, so we at least know it's off the rails? That's not strictly necessary, as the sha1 won't be in the repo, but it's a clue that it's a testing artifact vs one from a longer-term branch.

@tleedjarv
Copy link
Contributor Author

tleedjarv commented Feb 19, 2025

Added a commit for testing out the "unmerged-pr-" idea.

It's certainly ugly but it kind of works, I think.

@gdt
Copy link
Collaborator

gdt commented Feb 19, 2025

I checked an artifact and see unison-unmerged-pr-1124-git_979aeca3-ubuntu-x86_64 which looks good; it has the sha1 of the commit and the unmerged warning.

I am inclined to hit merge if you think it's ready.

@tleedjarv
Copy link
Contributor Author

I guess so. Might just as well merge.

@gdt gdt merged commit b46dd86 into bcpierce00:master Feb 20, 2025
31 checks passed
@tleedjarv tleedjarv deleted the gha-gitsha branch February 20, 2025 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants