Skip to content

Implement a merge freeze when an "[chore] Update core dependencies" PR on opentelemetry-collector-contrib is present #12837

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

Conversation

sAchin-680
Copy link
Contributor

Description

Extend the merge freeze logic in .github/workflows/scripts/check-merge-freeze.sh to include open PRs titled "[chore] Update core dependencies" in the opentelemetry-collector-contrib](https://github.com/open-telemetry/opentelemetry-collector-contrib repository, in addition to the existing check for "[chore] Prepare release" PRs in the core repo.

This ensures that no PRs can be merged into main while a core update PR is pending in contrib, helping us avoid version drift and keep dependencies aligned between core and contrib.

Link to tracking issue

Fixes #12830

Testing

Tested locally by:

  • Opening a dummy "[chore] Update core dependencies" PR on opentelemetry-collector-contrib
  • Running the updated check-merge-freeze.sh script to confirm it correctly blocks merges
  • Verified output message includes the contrib PR URL when such a PR is open

Documentation

No user-facing documentation changes needed. This change only affects internal CI logic.

@sAchin-680 sAchin-680 requested a review from a team as a code owner April 14, 2025 21:44
@sAchin-680 sAchin-680 requested a review from codeboten April 14, 2025 21:44
Copy link

linux-foundation-easycla bot commented Apr 14, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sAchin-680 / name: SACHIN KUMAR (f14c7ff)

@sAchin-680 sAchin-680 force-pushed the chore/merge-freeze-update-core-deps-pr branch from bcc414e to b5f796b Compare April 15, 2025 12:23
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.69%. Comparing base (e4f8b01) to head (f14c7ff).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12837   +/-   ##
=======================================
  Coverage   91.69%   91.69%           
=======================================
  Files         503      503           
  Lines       27550    27550           
=======================================
  Hits        25262    25262           
  Misses       1808     1808           
  Partials      480      480           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sAchin-680 sAchin-680 force-pushed the chore/merge-freeze-update-core-deps-pr branch 3 times, most recently from 31277db to a8c7fb7 Compare April 17, 2025 04:43
@sAchin-680
Copy link
Contributor Author

Hi @mx-psi !!

This PR #12837 extends the merge freeze logic to include [chore] Update core dependencies PRs from the opentelemetry-collector-contrib repository, helping maintain alignment between core and contrib during updates.

I've tested the changes locally and verified that the script correctly blocks merges when such PRs are open.

Would appreciate it if you could take a look whenever convenient. Thanks !

@sAchin-680 sAchin-680 force-pushed the chore/merge-freeze-update-core-deps-pr branch 4 times, most recently from 17d872a to 75c3aaa Compare April 21, 2025 09:39
@sAchin-680 sAchin-680 force-pushed the chore/merge-freeze-update-core-deps-pr branch from 75c3aaa to c884af3 Compare April 29, 2025 13:32
@mx-psi mx-psi requested a review from jade-guiton-dd April 30, 2025 10:21
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 reasonable to me, @jade-guiton-dd could you also take a look?

@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Apr 30, 2025

I have no issues with the PR on a technical standpoint, I think it should work.

However, I think that we may want to rethink exactly when we want to enact a CI-enforced merge freeze. If we decide that changes in core are problematic, not just for the "Prepare release" PR, but also for later steps in contrib, I don't think we should rely on one of two specific PRs being open, as these PRs are typically only open for two short fractions of the release process, so merges could easily "sneak" in between.

This is especially true since we only check for PRs by opentelemetrybot, and yet maintainers still regularly manually create "Prepare release" PRs in core (example last month), and especially "Update core dependencies" PRs in contrib (two examples for the previous release: example 1, example 2).

If non-maintainers merges in core are problematic at any point during the release process, I think we should enact a wider freeze that lasts the entire duration of the core/contrib release.

For example, it may make more sense to use the release issue (example), which is created along with the Prepare release PR, and is typically open during the whole process. We could split it into two issues, one for core/contrib, and one for operator/helm upgrades, and check the open status of the first one. Or we could set a label on the issue once core/contrib have been released, and check for that instead.

Ultimately, I think this is something to be decided by maintainers who have experience with the issues that merges during release can cause. And importantly, I think we should update the guidelines in the release doc to match whatever logic we decide to implement.

@songy23
Copy link
Member

songy23 commented Apr 30, 2025

and yet maintainers still regularly manually create "Prepare release" PRs in core (#12773), and especially "Update core dependencies" PRs in contrib (two examples for the previous release: open-telemetry/opentelemetry-collector-contrib#39695, open-telemetry/opentelemetry-collector-contrib#39697).

Yes we have to do this from time to time when the workflow fails. The automated PRs are in protected branches so may not always be possible to fix them in place.

@jade-guiton-dd
Copy link
Contributor

The release docs say that it should be possible by making a separate PR to merge into the bot branch:

If the PR needs updated in any way you can make the changes in a fork and PR those changes into the prepare-release-prs/x branch. You do not need to wait for the CI to pass in this prep-to-prep PR.

But I agree that there are probably legitimate use cases for manual prepare PRs, which is why I think we should change the criteria for the merge freeze. At the very least, removing the criterion that the PR we check for be authored by the bot.

@mx-psi
Copy link
Member

mx-psi commented Apr 30, 2025

I think we should merge this if we think this is a net improvement. I think it is, we may want to change things later in the process, but I don't think there is an obvious better alternative of doing this (the issues proposal sounds doable, but would need further changes of process).

@jade-guiton-dd @songy23 Do you think this is a net improvement? If so, I can merge this and update the release guidelines once we verify this works

@jade-guiton-dd
Copy link
Contributor

A thought occurs: we open "update core dependencies" PRs automatically every week, outside of releases. Are we sure we want to freeze core the whole time these are open?

@songy23
Copy link
Member

songy23 commented Apr 30, 2025

Yes, I agree. The fact that we need to manually create those PRs is a different issue.

making a separate PR to merge into the bot branch:

The workflow itself fails from time to time, e.g. http://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/14602074902/job/40962393942

@mx-psi
Copy link
Member

mx-psi commented Apr 30, 2025

A thought occurs: we open "update core dependencies" PRs automatically every week, outside of releases. Are we sure we want to freeze core the whole time these are open?

I personally see it as a necessary part of the release. We did discuss this in a thread on #otel-collector-core-leads and got some upvotes/support https://cloud-native.slack.com/archives/C07QV8LGV36/p1737736010945599?thread_ts=1737734591.519929&cid=C07QV8LGV36 so I think we should at least give it a try

@sAchin-680 sAchin-680 force-pushed the chore/merge-freeze-update-core-deps-pr branch from 379e250 to f14c7ff Compare May 1, 2025 05:06
@sAchin-680
Copy link
Contributor Author

Thanks a lot @mx-psi, @songy23, and @jade-guiton-dd — really appreciate all the feedback and support on this. especially the discussions around improving the merge freeze logic was supportive in shaping this PR. Cheers!

@songy23 songy23 added ready-to-merge Code review completed; ready to merge by maintainers Skip Contrib Tests ci-cd CI, CD, testing, build issues labels May 1, 2025
@songy23 songy23 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label May 1, 2025
@mx-psi mx-psi added this pull request to the merge queue May 5, 2025
Merged via the queue into open-telemetry:main with commit 73e7c97 May 5, 2025
65 of 68 checks passed
github-merge-queue bot pushed a commit that referenced this pull request May 7, 2025
…2988)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Updates release guidelines to reflect #12837

---------

Co-authored-by: Chao Weng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-cd CI, CD, testing, build issues ready-to-merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry Skip Contrib Tests
Projects
None yet
4 participants