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

set associated head sha on pull request event #234

Merged
merged 1 commit into from Oct 7, 2022

Conversation

crazy-max
Copy link
Member

fixes #206

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@crazy-max crazy-max marked this pull request as ready for review October 7, 2022 22:35
@crazy-max crazy-max merged commit 8ed470c into docker:master Oct 7, 2022
@crazy-max crazy-max deleted the fix-sha-pr branch October 7, 2022 22:36
@gustavovalverde
Copy link

This might seem as a fix in versioning, but it's a breaking change. For example, this breaks all our workflows which depend on the built image sha

Now we have to include extra conditions as the sha won't be the same when there's a PR and a push to main. It would have been nice to increase the version to 5.0.0 considering this would break most scenarios being based on GITHUB_SHA or ${{ github.sha }}

This now requires implementations to use ( ${{ github.event.pull_request.head.sha }} || {{ github.sha }} ) for a single pull to work on both GitHub events (PRs and push)

@crazy-max
Copy link
Member Author

@gustavovalverde From what I see on ZcashFoundation/zebra#5390, you're pushing an image on PR and others jobs depend on it using github.sha as tag.

I didn't think about this use case as pushing on PR is not possible in majority of cases as secrets can't be exposed. But in your case I see you're using google-github-actions/auth action to connect to your provider using the OIDC token so you're able to push to its registry.

Even if you can share image between jobs without pushing them, it still sounds like a valid use case to use a registry. I will revert this change, sorry about that.

@@ -37,6 +37,10 @@ export class Meta {
context.ref = `refs/pull/${context.payload.number}/merge`;
}

if ((/pull_request/.test(context.eventName) || /pull_request_target/.test(context.eventName)) && context.payload?.pull_request?.head?.sha != undefined) {
Copy link

Choose a reason for hiding this comment

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

This appears to check that substring “pull_request” appears anywhere in the event name. Maybe the intention was to check if the name starts with a certain substring? Or matches exactly if that’s the entire event name?

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.

Use correct commit sha for PRs?
3 participants