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] Fix check merge freeze job #12045

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Jan 7, 2025

To unblock #12043, which is always rejected on the queue.

Apparently, github.event.merge_group.head_commit.author.name is OpenTelemetry Bot not opentelemetrybot. See https://github.com/open-telemetry/opentelemetry-collector/actions/runs/12658978490/job/35277057933.

Since github.event.merge_group.head_commit.author doesn't have login field, I've switched the condition to use the name field on both github.event.pull_request.user and github.event.merge_group.head_commit.author

@dmitryax dmitryax requested a review from a team as a code owner January 7, 2025 19:48
@dmitryax dmitryax requested a review from djaglowski January 7, 2025 19:48
@dmitryax dmitryax changed the title [chore] Fix check merge freeze job [chore] Fix Check merge freeze job Jan 7, 2025
@dmitryax dmitryax changed the title [chore] Fix Check merge freeze job Fix Check merge freeze job Jan 7, 2025
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.67%. Comparing base (286b2f5) to head (439426f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12045   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files         455      455           
  Lines       24038    24038           
=======================================
  Hits        22037    22037           
  Misses       1629     1629           
  Partials      372      372           

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

@dmitryax dmitryax changed the title Fix Check merge freeze job [chore] Fix check merge freeze job Jan 7, 2025
@dmitryax dmitryax force-pushed the fix-check-merge-freeze branch from a46c9b8 to 9b68d59 Compare January 7, 2025 19:56
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.

@dmitryax dmitryax force-pushed the fix-check-merge-freeze branch from 9b68d59 to 4e7a8f0 Compare January 7, 2025 19:57
@songy23 songy23 added release release:retro Issues discussed in a release retrospective Skip Contrib Tests labels Jan 7, 2025
@dmitryax
Copy link
Member Author

dmitryax commented Jan 7, 2025

I verified the condition works now

@songy23
Copy link
Member

songy23 commented Jan 7, 2025

maybe close #12043 for now and reopen later?

@dmitryax dmitryax force-pushed the fix-check-merge-freeze branch from 4e7a8f0 to 94a8f03 Compare January 7, 2025 20:06
@dmitryax dmitryax closed this Jan 7, 2025
@dmitryax dmitryax reopened this Jan 7, 2025
@dmitryax dmitryax marked this pull request as draft January 7, 2025 20:12
@dmitryax dmitryax force-pushed the fix-check-merge-freeze branch from 94a8f03 to 439426f Compare January 7, 2025 20:52
@dmitryax dmitryax marked this pull request as ready for review January 7, 2025 20:52
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

just checking the description mentions the merge_group event twice, but the change here is on the pull_request event as well, is this just a typo in the description?

@dmitryax
Copy link
Member Author

dmitryax commented Jan 7, 2025

just checking the description mentions the merge_group event twice, but the change here is on the pull_request event as well, is this just a typo in the description?

Just typo. Thanks for catching. Updated

@dmitryax dmitryax added this pull request to the merge queue Jan 7, 2025
Merged via the queue into open-telemetry:main with commit 7b804b5 Jan 7, 2025
44 of 45 checks passed
@dmitryax dmitryax deleted the fix-check-merge-freeze branch January 7, 2025 21:44
dmitryax added a commit to dmitryax/opentelemetry-collector that referenced this pull request Jan 7, 2025
In open-telemetry#12045, I assumed that `github.event.pull_request.user.name` would be present, but apparently it's not. So we need to switch back to using `github.event.pull_request.user.login`
@jade-guiton-dd
Copy link
Contributor

Sorry for all the trouble @dmitryax... We weren't able to test the merge group check independently, but I should probably have seen this coming.

@dmitryax
Copy link
Member Author

dmitryax commented Jan 11, 2025

@jade-guiton-dd, no worries. I realize it's hard to test without trying. Thanks for all the improvements!

sfc-gh-sili pushed a commit to sfc-gh-sili/opentelemetry-collector that referenced this pull request Jan 15, 2025
To unblock
open-telemetry#12043,
which is always rejected on the queue.

Apparently, `github.event.merge_group.head_commit.author.name` is
`OpenTelemetry Bot` not `opentelemetrybot`. See
https://github.com/open-telemetry/opentelemetry-collector/actions/runs/12658978490/job/35277057933.

Since `github.event.merge_group.head_commit.author` doesn't have `login`
field, I've switched the condition to use the `name` field on both
`github.event.pull_request.user` and
`github.event.merge_group.head_commit.author`
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 <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
Labels
release:retro Issues discussed in a release retrospective release Skip Contrib Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants