Label release:merge-freeze should not block the release PR #11906
Assignees
Labels
Comments
|
Hypothesis: This was a race condition where the check workflow started when the PR is created, but before the label was added, validating the |
This was referenced Dec 17, 2024
github-merge-queue bot
pushed a commit
that referenced
this issue
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](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 #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.
HongChenTW
pushed a commit
to HongChenTW/opentelemetry-collector
that referenced
this issue
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.
mx-psi
added a commit
to open-telemetry/opentelemetry-collector-contrib
that referenced
this issue
Jan 15, 2025
#### Description As stated in the [Release Procedure document](https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/release.md#releasing-opentelemetry-collector-contrib), all merging in Contrib should be halted while the "Prepare release" PR is open. This PR adds a CI check which fails if such a release PR is currently open. This is the same as the CI check introduced in Core as part of [this issue](open-telemetry/opentelemetry-collector#11707) ([Initial PR](open-telemetry/opentelemetry-collector#11744), [bug](open-telemetry/opentelemetry-collector#11808), [fix](open-telemetry/opentelemetry-collector#11849), [bug 2](open-telemetry/opentelemetry-collector#11906), [fix 2](open-telemetry/opentelemetry-collector#11936), [fix 3](open-telemetry/opentelemetry-collector#12045), [fix 4](open-telemetry/opentelemetry-collector#12051)). Like its predecessor, this will only be fully effective once the merge queue is enabled on this repo (see #36788 for progress on that), enabling us to do proper last-minute checks instead of relying on the freeze status at the time of the latest PR update. Similarly, for proper enforcement, this check will need to be marked as required in the main branch protections (I will create an issue on the community repo to that effect once this PR is merged). #### Link to tracking issue Updates #36848 #### Testing Considering the multiple iterations this code went through on Core, it should now work properly without adaptation. However, considering the multiple iterations this code went through on Core, we should expect the unexpected, especially when it comes to the interaction with the merge queue, so release managers will need to be aware of this change, in case it breaks the release process. --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> Co-authored-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com>
chengchuanpeng
pushed a commit
to chengchuanpeng/opentelemetry-collector-contrib
that referenced
this issue
Jan 26, 2025
…emetry#37097) #### Description As stated in the [Release Procedure document](https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/release.md#releasing-opentelemetry-collector-contrib), all merging in Contrib should be halted while the "Prepare release" PR is open. This PR adds a CI check which fails if such a release PR is currently open. This is the same as the CI check introduced in Core as part of [this issue](open-telemetry/opentelemetry-collector#11707) ([Initial PR](open-telemetry/opentelemetry-collector#11744), [bug](open-telemetry/opentelemetry-collector#11808), [fix](open-telemetry/opentelemetry-collector#11849), [bug 2](open-telemetry/opentelemetry-collector#11906), [fix 2](open-telemetry/opentelemetry-collector#11936), [fix 3](open-telemetry/opentelemetry-collector#12045), [fix 4](open-telemetry/opentelemetry-collector#12051)). Like its predecessor, this will only be fully effective once the merge queue is enabled on this repo (see open-telemetry#36788 for progress on that), enabling us to do proper last-minute checks instead of relying on the freeze status at the time of the latest PR update. Similarly, for proper enforcement, this check will need to be marked as required in the main branch protections (I will create an issue on the community repo to that effect once this PR is merged). #### Link to tracking issue Updates open-telemetry#36848 #### Testing Considering the multiple iterations this code went through on Core, it should now work properly without adaptation. However, considering the multiple iterations this code went through on Core, we should expect the unexpected, especially when it comes to the interaction with the merge queue, so release managers will need to be aware of this change, in case it breaks the release process. --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> Co-authored-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See an example in #11904
The text was updated successfully, but these errors were encountered: