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

OSOE-517: Add merge queue support for verifying GitHub Action expected refs #292

Merged
merged 5 commits into from
Dec 9, 2023

Conversation

davidpuplava
Copy link
Contributor

@davidpuplava davidpuplava commented Dec 3, 2023

Adds support for merge queues to run the validate-this-gha-refs workflow when a pull request is added to the merge group, if the repository is configured for it.

Adds a skipReviewApproval flag to the Determine Expected Ref for GitHub Actions Files step of the validate-this-gha-refs workflow that changes the default behavior of expecting the base branch ref (e.g. @dev) when the Pull Request is approved (using GitHub's pull request review feature). This allows additional commits on the PR branch after review approval but before adding the PR to the merge queue.

Here is a reminder on how the validate-this-gha-refs currently behaves:

  1. When a PR is submitted and/or has additional commits, and prior to PR Approval, the expected refs are the target branch e.g. @issue/[OSOE-517](https://lombiq.atlassian.net/browse/OSOE-517).
  2. After PR Review approval, the expected refs are the base branch e.g. @dev so that the developer or reviewer can make sure the refs are changed back to dev prior to merging the pull request.

Here is how this PR changes the validate-this-gha-refs behavior:

  1. Same as above. When a PR is submitted and/or has additional commits, and prior to PR Approval, the expected refs are the target branch e.g. @issue/[OSOE-517](https://lombiq.atlassian.net/browse/OSOE-517).
  2. If and only if, the skipReviewApproval flag is set to False, will the behavior be the same as above. After PR approval, the expected refs are the base branch e.g. @dev so that the developer or reviewer can make sure the refs are changed back to dev prior to merging the pull request.
  3. For LGHA, skipReviewApproval flag is set to True to align with how PRs can get a PR Review approval but receive additional commits before wanting to enforce the base branch refs, e.g. @dev.
  4. Enforcing the base branch ref (e.g. @dev) happens after the PR is added to the merge queue (i.e. when the "Merge When Ready" button is clicked. NOTE I believe for this to work, you have to configure the merge queue to run the validate-this-gha-refs workflow as a status check.

This PR essentially changes the "signal" for approval from PR Review Approval to adding it to the Merge Queue (clicking the Merge When Ready button).

@DemeSzabolcs
Copy link
Member

@davidpuplava
Copy link
Contributor Author

Ok, I'll take a look - for some reason the head and base refs are both empty which is unexpected.

@davidpuplava
Copy link
Contributor Author

I approved the PR but we got this error here: https://github.com/Lombiq/GitHub-Actions/actions/runs/7092583727/job/19304192097?pr=292#step:11:27

I fixed the above error.

In consolidating the Verify GitHub Actions Items Match Expected Ref step, I overlooked that on a pull_request_review event, the head and base refs come from a different context than the github context. This if/else block in the Determine Expected Ref for GitHub Actions Files now covers the PR approval and Merge Group approval cases.

@DemeSzabolcs
Copy link
Member

Okay, I approved it, now I will try to merge it and it should fail, because I haven't changed the action branches'.

@DemeSzabolcs DemeSzabolcs added this pull request to the merge queue Dec 9, 2023
Merged via the queue into dev with commit 6a26588 Dec 9, 2023
5 checks passed
@DemeSzabolcs
Copy link
Member

DemeSzabolcs commented Dec 9, 2023

@davidpuplava
Well the validate GHA refs failed: https://github.com/Lombiq/GitHub-Actions/actions/runs/7151812876
But it still merged the branch:
image

Shouldn't this also prevent the merge?

(BTW I reverted the merge for now. You can open another PR with the same branch.)

@davidpuplava
Copy link
Contributor Author

@DemeSzabolcs I think this might be a settings issue because I don't see a workflow action run triggered by the add to the merge queue. Under All workflows on the Actions tab, we should a Validate GitHub Actions Refs #XX: Merge group checks requested but I don't see it in LGHA's repo. Here is a screenshot from my test repo:
image

Can you check and confirm the branch protection rule for dev has the Require status checks to pass before merging and that validate-gha-refs is listed under the Status checks that are required? Here is a screenshot of my test repo that uses merge queues with the required status checks.
image

The workflow run where the validate GHA refs failed (https://github.com/Lombiq/GitHub-Actions/actions/runs/7151812876) is from the push to the dev branch which also triggers a check to make sure all the refs point back to @dev.

@DemeSzabolcs
Copy link
Member

Thank you, I see! I can't check that. @BenedekFarkas, or @Piedone could you please check it?

Can you check and confirm the branch protection rule for dev has the Require status checks to pass before merging and that validate-gha-refs is listed under the Status checks that are required? Here is a screenshot of my test repo that uses merge queues with the required status checks.

@BenedekFarkas
Copy link
Member

Thanks, I've just set this up for this repo. Please confirm that this means that after approving the PR, validate-gha-refs will still fail if any of the workflow branch references aren't pointing to dev, but it won't allow merging the PR because of that, only after restoring the branch references to dev and the check is successful.

@DemeSzabolcs
Copy link
Member

@davidpuplava Please open another PR, with the same changes as this one and we can test it.

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.

3 participants