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

[chore] Merge freeze check uses title to find Release PRs #11936

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Dec 17, 2024

Description

The last attempt at making the Merge freeze check work in release PRs failed (#11906). This PR tries a different approach: it changes the criteria of the Merge freeze check, so that a freeze is enacted iff there is an open PR authored by @opentelemetrybot whose title contains "[chore] Prepare release" (note that if it weren't for the author, this PR would qualify).

This PR additionally reverts #11849, so no label is added to the release PR. I also added the pull_request.enqueued trigger, taking inspiration from Merge Freeze, to see if it could help reject PRs earlier.

I tried to make sure the freeze check would be properly skipped for the release PR itself, both in PR checks and in the merge queue, but given the state of Github's documentation, I'm not very confident about this.

Notably, these is an edge case where I'm not sure what would happen: what if another PR gets added to the merge queue at the same time as the release PR? How many times would the "merge_group" check run, and with what values for "github.event.merge_group.head_commit"? Would both PR be booted out of the queue (not great)? Would both be accepted (way worse)? Does it depend on the order?

Link to tracking issue

Fixes #11906 and fixed #11808

Testing

As always with this, it's pretty much impossible to test before merging. Once merged, I strongly recommend we do the following test to make sure that this issue does not block the real release process again:

  • Create two dummy PRs that change nothing of consequence: the freeze check should pass
  • Run the "Prepare release" action
  • Rerun the freeze check on one of the dummy PRs: it should now fail
  • Approve the second PR and try to merge it: it should be booted out of the merge queue
  • Close all test PRs

This unfortunately does not test whether the release PR gets merged properly, but I don't see how to test until the next release process, unfortunately.

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.59%. Comparing base (4fc50a8) to head (2471691).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11936   +/-   ##
=======================================
  Coverage   91.59%   91.59%           
=======================================
  Files         448      448           
  Lines       23759    23761    +2     
=======================================
+ Hits        21761    21763    +2     
  Misses       1623     1623           
  Partials      375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

This looks good overall, I can take care of the testing

.github/workflows/prepare-release.yml Show resolved Hide resolved
@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review December 17, 2024 14:08
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner December 17, 2024 14:08
@jade-guiton-dd jade-guiton-dd changed the title [chore] Fix "Prepare release" action again [chore] Merge freeze check uses title to find Release PRs Dec 17, 2024
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

This makes sense to me

@codeboten codeboten added this pull request to the merge queue Dec 17, 2024
Merged via the queue into open-telemetry:main with commit ba6b7ee Dec 17, 2024
69 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 17, 2024
This was referenced Dec 18, 2024
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this pull request Dec 19, 2024
…etry#11936)

#### Description

The last attempt at making the Merge freeze check work in release PRs
failed (open-telemetry#11906). This PR tries a different approach: it changes the
criteria of the Merge freeze check, so that a freeze is enacted iff
there is an open PR authored by @opentelemetrybot whose title contains
"[chore] Prepare release" (note that if it weren't for the author, this
PR would qualify).

This PR additionally reverts open-telemetry#11849, so no label is added to the release
PR. I also added the `pull_request.enqueued` trigger, taking inspiration
[from Merge
Freeze](https://docs.mergefreeze.com/github-merge-queue#how-it-works),
to see if it could help reject PRs earlier.

I tried to make sure the freeze check would be properly skipped for the
release PR itself, both in PR checks and in the merge queue, but given
the state of Github's documentation, I'm not very confident about this.

Notably, these is an edge case where I'm not sure what would happen:
what if another PR gets added to the merge queue at the same time as the
release PR? How many times would the "merge_group" check run, and with
what values for "github.event.merge_group.head_commit"? Would both PR be
booted out of the queue (not great)? Would both be accepted (way worse)?
Does it depend on the order?

#### Link to tracking issue
Fixes open-telemetry#11906 and fixed open-telemetry#11808

#### Testing
As always with this, it's pretty much impossible to test before merging.
Once merged, I strongly recommend we do the following test to make sure
that this issue does not block the real release process again:
- Create two dummy PRs that change nothing of consequence: the freeze
check should pass
- Run the "Prepare release" action
- Rerun the freeze check on one of the dummy PRs: it should now fail
- Approve the second PR and try to merge it: it should be booted out of
the merge queue
- Close all test PRs

This unfortunately does not test whether the release PR gets merged
properly, but I don't see how to test until the next release process,
unfortunately.
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.

Label release:merge-freeze should not block the release PR "Prepare Release" action fails to add label
4 participants