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: Re-add support for merge group approval when verifying GHA refs #299

Merged
merged 7 commits into from
Dec 14, 2023

Conversation

davidpuplava
Copy link
Contributor

Copied from prior PR #292

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).

@github-actions github-actions bot changed the title Re-add support for merge group approval when verifying GHA refs OSOE-517: Re-add support for merge group approval when verifying GHA refs Dec 12, 2023
@DemeSzabolcs DemeSzabolcs added this pull request to the merge queue Dec 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 13, 2023
@DemeSzabolcs
Copy link
Member

Okay, I tried to merge it and it failed as it should.

@DemeSzabolcs
Copy link
Member

Well it works, but I will need your help with merging this @BenedekFarkas @Piedone.

Since if the workflow references the action in the issue branch, it will fail as it should (so it doesn't allow me to merge it). But if I change the reference to dev it still fails of course, because the action is not yet in dev so it can't find it. This should be a one off problem though.

So the solution would be to turn off the "required" check, merge this, then re-enable it.

@davidpuplava
Copy link
Contributor Author

So the solution would be to turn off the "required" check, merge this, then re-enable it.

@DemeSzabolcs One thing you may want to try is to leave the Check Merge Queue Adds step ref as @issue/OSOE-517 and try adding it back to the merge queue.

This will allow the Workflow action to run without encountering the missing action error seen in this run.

What I expect will happen is that the merge will succeed, but then the push to the dev branch will fail saying you need to update that ref to @dev but at this point the new action will be in the dev branch.

The reason I think this will work is because we have this Determine Diff Filter step that will decide whether to include the "added files" as part of the ref check.

It's not ideal but I think it's the only way to work around new actions or workflows not yet existing in the target branch.

@DemeSzabolcs DemeSzabolcs added this pull request to the merge queue Dec 14, 2023
Merged via the queue into dev with commit e0da351 Dec 14, 2023
4 checks passed
@DemeSzabolcs
Copy link
Member

Thank you, it worked! I will open another PR and now switch it back to dev.

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