Skip to content

[chore] Add CI check to enforce merge freezes #11744

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

Merged
merged 5 commits into from
Nov 28, 2024

Conversation

jade-guiton-dd
Copy link
Contributor

@jade-guiton-dd jade-guiton-dd commented Nov 25, 2024

Description

As stated in the Release Procedure document, all merging in Core should be halted while the "Prepare release" PR is open. This PR adds a CI job checking the existence of such a PR, allowing us to enforce these freezes automatically.

To make things simple, I decided to distinguish the "Prepare release" PR from others by using a new prepare:merge-freeze label. I modified the "Prepare release" action to include that label on the automatically created PR, but in rare cases where maintainers might create the PR themselves, the label can be added manually. Triagers will need to be careful not to accidentally add this label to any random PR to avoid artificial freezes.

Because, in the current state, this CI check is run on each commit of a PR, and at no other time, there is a chance for:

  • False positives: If a commit is pushed during a freeze, and the freeze is later rescinded, the PR will remain unmergeable until a new commit is pushed. A simple solution is to re-run the check manually.
  • False negatives: If a commit is pushed outside of a freeze, then a freeze is enacted, the PR will remain mergeable despite the freeze. This is more problematic.

To solve this second problem and make this check truly useful, two changes in the repository configuration are necessary:

  • the merge queue should be enabled, allowing us to run checks again just before merging;
  • this new CI check should be added to the list of required checks for the main branch.

This will allow us to automatically cancel the merging of PRs during a freeze. A later PR on the community repository should take care of these configuration changes.

Note that another solution to handle outdated CI checks would be to use a tool like MergeFreeze, but reading this document, it sounds like it would be a harder sell with the community than enabling the merge queue.

Link to tracking issue

Updates #11707

Testing

Simple tests were made on a separate repository to check that enabling the merge queue fixes the false negatives.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.54%. Comparing base (f74890a) to head (bc23717).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11744      +/-   ##
==========================================
- Coverage   91.61%   91.54%   -0.07%     
==========================================
  Files         443      443              
  Lines       23770    23764       -6     
==========================================
- Hits        21776    21754      -22     
- Misses       1620     1637      +17     
+ Partials      374      373       -1     

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

@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review November 25, 2024 16:40
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner November 25, 2024 16:40
@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2024

False positives: If a commit is pushed during a freeze, and the freeze is later rescinded, the PR will remain unmergeable until a new commit is pushed. A simple solution is to re-run the check manually.

As I understand it this problem remains even after we make the repository changes. Should we explicitly encourage re-running the check in the error message so that people know how to deal with false positives?

@jade-guiton-dd
Copy link
Contributor Author

jade-guiton-dd commented Nov 26, 2024

As I understand it this problem remains even after we make the repository changes. Should we explicitly encourage re-running the check in the error message so that people know how to deal with false positives?

That's probably a good idea.

@jade-guiton-dd
Copy link
Contributor Author

I went with release:merge-freeze for the triggering label in the end.

@mx-psi
Copy link
Member

mx-psi commented Nov 26, 2024

@open-telemetry/collector-approvers This adds a check to prevent merging other PRs during merge freezes (step 3 of the release process), which will be more useful after we have a merge queue on this repository. I will merge this on Thursday EU morning unless somebody objects before then.

@mx-psi mx-psi merged commit b52e39c into open-telemetry:main Nov 28, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 28, 2024
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request 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 <[email protected]>
Co-authored-by: Moritz Wiesinger <[email protected]>
chengchuanpeng pushed a commit to chengchuanpeng/opentelemetry-collector-contrib that referenced this pull request 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 <[email protected]>
Co-authored-by: Moritz Wiesinger <[email protected]>
zeck-ops pushed a commit to zeck-ops/opentelemetry-collector-contrib that referenced this pull request Apr 23, 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 <[email protected]>
Co-authored-by: Moritz Wiesinger <[email protected]>
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