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

Use the GH webhook payload to parse PR details #4817

Merged
merged 4 commits into from
Jun 15, 2020

Conversation

praneetloke
Copy link
Contributor

@praneetloke praneetloke commented Jun 12, 2020

Related to #4758 and #4613.

In a previous PR, I added the CI env vars detection for GH Actions. Unfortunately, it looks like there is some problem with the values of GITHUB_REF and GITHUB_SHA for a PR. It looks like the example shown in the doc is not what we are actually seeing. To demonstrate this, I opened a draft PR in our docs repo, which has an extra step to print out the env vars and the event JSON separately. Note how the value of the GITHUB_REF value does not match the documented example of ref/heads/<branch name>. And for GITHUB_SHA -- I actually can't even tell what this SHA is. It doesn't look like it is the value of anything that I can correlate. Perhaps I read the docs incorrectly and misunderstood what they were supposed to be.

In any case, if you look at the event JSON, the values that we are looking to use are correctly specified in there, including the pull_request.head.ref, which has the PR branch name and the SHA value in pull_request.head.sha.

In addition to the problem with the values of the two env vars, the PR number extraction was incorrect. In the previous PR, I was trying to unmarshal the value of the GITHUB_EVENT_PATH directly, which will never work since it is just the path to the JSON file that actually contains the event. This would have only caused us to not set the ci.pr.number env metadata key. I have fixed this as well now.

@praneetloke praneetloke self-assigned this Jun 12, 2020
@chrsmith
Copy link
Contributor

So the behavior from the GitHub Actions sounds like it makes sense to me, or at least seems reasonable.

Unfortunately, it looks like there is some problem with the values of GITHUB_REF and GITHUB_SHA for a PR. It looks like the example shown in the doc is not what we are actually seeing. To demonstrate this, I opened a draft PR in our docs repo, which has an extra step to print out the env vars and the event JSON separately.

The GITHUB_REF env var was "refs/pull/3584/merge", which while obviously not as helpful as the docs, does imply that it is running on the merge commit that would be generated if the pull request were merged with the target branch.

So that's probably why you couldn't associate the SHA with anything, because it's not the SHA of the pull request branch, but the "pull request branch + target branch".

The [GitHub event(https://github.com/pulumi/docs/pull/3584/checks?check_run_id=766615693#step:13:7) printed a "synchronize" action, which if I recall correctly is what happens whenever a push is made to a branch that is tracked by a pull request.

The Pulumi GitHub app listens for those and writes them into the GitCommitListeners table, here. (Relevant GitHub docs for context.)

In any case, if you look at the event JSON, the values that we are looking to use are correctly specified in there, including the pull_request.head.ref, which has the PR branch name and the SHA value in pull_request.head.sha.

That sounds right. And appears to be what the Pulumi GitHub App is doing when it sees the "synchronize" event as well. (Code).

Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

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

Thanks for tracking this down Praneet! Definitely a pain, double-checking the output you saw from the GitHub Actions at runtime, this does look like the right way to gather this information.

The only suggestion is that maybe we would want to confirm the githubActionsPullRequestEvent action field? Since there are a bunch of different ones, and potentially the way we would interpret the SHA/branch might be different? (But it doesn't look like the case, since I doubt the workflow would trigger in response to the GitHub PR having review_requested or being locked, etc.)

:shipit: !

@praneetloke
Copy link
Contributor Author

The GITHUB_REF env var was "refs/pull/3584/merge", which while obviously not as helpful as the docs, does imply that it is running on the merge commit that would be generated if the pull request were merged with the target branch.

Correct. The values of the env var as-is makes sense to me as well. I am just not sure when GITHUB_REF would ever be refs/heads/feature-branch as the docs demonstrate? One thing is for sure -- it doesn't seem to have that value when a PR is opened or updated, which is what matters to the CLI. Moreover, the GITHUB_SHA doesn't actually even look like the merge commit SHA, if you look at the webhook payload. See pull_request.merge_commit_sha. It's null when a PR is opened, but has a value for subsequent PR synchronize actions. which does not match the value of GITHUB_SHA. I am not sure if I am missing something?

The GitHub event printed a "synchronize" action, which if I recall correctly is what happens whenever a push is made to a branch that is tracked by a pull request.

Correct. I pasted the link to the GHA execution for the synchronize action. Here's the opened action for that same PR. (I had opened that PR by removing all of the original steps before I restored them in a subsequent push.)

The only suggestion is that maybe we would want to confirm the githubActionsPullRequestEvent action field? Since there are a bunch of different ones, and potentially the way we would interpret the SHA/branch might be different? (But it doesn't look like the case, since I doubt the workflow would trigger in response to the GitHub PR having review_requested or being locked, etc.)

Yeah it doesn't seem to be the case. The CLI would just need to take whatever value is in the PR webhook payload from the workflow environment. I do have a check that we would parse the webhook payload event only if GITHUB_EVENT_NAME is pull_request.

@praneetloke praneetloke merged commit 2513709 into master Jun 15, 2020
@pulumi-bot pulumi-bot deleted the praneetloke/github-actions-ci-2 branch June 15, 2020 19:49
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.

None yet

2 participants