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.

Sign up for GitHub

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

Jump to bottom

"Prepare Release" action fails to add label #11808

Closed
jade-guiton-dd opened this issue Dec 5, 2024 · 9 comments · Fixed by #11849 or #11936
Closed

"Prepare Release" action fails to add label #11808

jade-guiton-dd opened this issue Dec 5, 2024 · 9 comments · Fixed by #11849 or #11936
Labels
discussion-needed Community discussion needed release:blocker The issue must be resolved before cutting the next release release:retro Issues discussed in a release retrospective

Comments

@jade-guiton-dd
Copy link
Contributor

I recently merged a PR (#11744) to add a CI check which prevents PRs from being merged in main while a "Prepare Release" PR is open, enforcing the merge freeze described in the release procedure. To detect the Prepare Release PR, I decided to rely on a new "release:merge-freeze" label, to be added automatically to said PR.

However, during the latest release, the "Prepare Release" action failed, with the message "opentelemetrybot does not have permission to update the pull request". The PR was still created, but the label was not added, presumably because the OPENTELEMETRYBOT_GITHUB_TOKEN used by the action does not have the permissions to add labels to PRs.

I see two possible solutions:

  1. Generate a new token with the appropriate additional permission;
  2. Have the bot comment on the PR / add something in the description that triggers another workflow which adds the label.
@jade-guiton-dd jade-guiton-dd added the discussion-needed Community discussion needed label Dec 5, 2024
@mx-psi mx-psi added the release:blocker The issue must be resolved before cutting the next release label Dec 5, 2024
@atoulme atoulme added the release:retro Issues discussed in a release retrospective label Dec 6, 2024
@atoulme
Copy link
Contributor

atoulme commented Dec 6, 2024

  1. seems like the painless option.

@mx-psi
Copy link
Member

mx-psi commented Dec 10, 2024

I am in favor of 1 as well

@mx-psi
Copy link
Member

mx-psi commented Dec 10, 2024

@open-telemetry/collector-approvers does (1) look good to you?

@mx-psi
Copy link
Member

mx-psi commented Dec 10, 2024

Looking at https://github.com/open-telemetry/community/blob/main/assets.md#opentelemetry-bot it may be hard to do (1) given that this is an OpenTelemetry-wide token. So we may want to look into an alternative solution

@jade-guiton-dd
Copy link
Contributor Author

jade-guiton-dd commented Dec 10, 2024 •
edited
Loading

I can try using a regular CI token instead of the OpenTelemetry bot one for the part where we add the label, and we'll see if that works.

@jade-guiton-dd jade-guiton-dd mentioned this issue Dec 11, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 12, 2024
@jade-guiton-dd
[chore] Fix "Prepare Release" action (#11849)
220b583 
#### Description

The "Prepare Release" action failed during the latest release process,
as the OpenTelemetry Bot, which opens the release PR, does not have the
permissions to add the new `release:merge-freeze` label. This PR fixes
this by creating the PR with the bot token, then adding the label
separately using the regular CI token.

Since we already use default CI tokens on -contrib to add labels, I
assume they have the permission to do that on this repo as well. I
deemed this simpler than the alternative solutions mentioned in the
tracking issue, though I'm not certain it will work.

Note that the tracking issue is a release blocker.

#### Link to tracking issue
Fixes #11808

#### Testing
I'm not sure how I could test this without getting it merged first
unfortunately. Once merged, we could try running the action, then
closing the resulting PR and issue.
@jade-guiton-dd
Copy link
Contributor Author

jade-guiton-dd commented Dec 17, 2024 •
edited
Loading

The idea to add the label using the regular CI token instead of the OTelBot token did not work out. The label was properly added, but the release PR ended up being blocked by itself:

  • The initial run of the check on PR creation wasn't skipped because the PR did not have the label at first, but by the time the check actually ran, it DID have the label, causing the check to fail.
  • The check should have been re-run once the label was added, solving the issue, but events caused by CI do not trigger CI (in fact, that is the entire raison d'être of the OTelBot)...

@mx-psi
Copy link
Member

mx-psi commented Dec 17, 2024

Would it improve things if we base things on the PR title/description? e.g. something like "if there is a PR that has a name with this thing in it and the author is opentelemetrybot" as the condition for a freeze. This is:

  • something the PR should have from the very beginning
  • ~same level of access (not everybody can change the release prep PR title)
  • no additional permissions needed from the bot

@jade-guiton-dd
Copy link
Contributor Author

Right, that is what I was about to suggest. The other solution would be to try to convince the Technical Committee to give the OTel Bot permission to add labels, but that might not be worth the effort. The only issue I see with this is that we need to make sure release managers only ever use the bot to create the PR, but there's only one semi-recent instance of a release PR being created manually (#10190), so it's probably fine.

@mx-psi
Copy link
Member

mx-psi commented Dec 17, 2024

The only issue I see with this is that we need to make sure release managers only ever use the bot to create the PR, but there's only one semi-recent instance of a release PR being created manually (#10190), so it's probably fine.

I think a bot-only release is what we should be strive for so I don't see a big issue with that 🤖

@jade-guiton-dd jade-guiton-dd mentioned this issue Dec 17, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 17, 2024
@jade-guiton-dd
[chore] Merge freeze check uses title to find Release PRs (#11936)
ba6b7ee 
#### 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
@jade-guiton-dd @HongChenTW
[chore] Fix "Prepare Release" action (open-telemetry#11849)
aa4e5b2 
#### Description

The "Prepare Release" action failed during the latest release process,
as the OpenTelemetry Bot, which opens the release PR, does not have the
permissions to add the new `release:merge-freeze` label. This PR fixes
this by creating the PR with the bot token, then adding the label
separately using the regular CI token.

Since we already use default CI tokens on -contrib to add labels, I
assume they have the permission to do that on this repo as well. I
deemed this simpler than the alternative solutions mentioned in the
tracking issue, though I'm not certain it will work.

Note that the tracking issue is a release blocker.

#### Link to tracking issue
Fixes open-telemetry#11808

#### Testing
I'm not sure how I could test this without getting it merged first
unfortunately. Once merged, we could try running the action, then
closing the resulting PR and issue.
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this issue Dec 19, 2024
@jade-guiton-dd @HongChenTW
[chore] Merge freeze check uses title to find Release PRs (open-telem…
d218005 
…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.
@jade-guiton-dd jade-guiton-dd mentioned this issue Jan 8, 2025
Merged
mx-psi added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this issue Jan 15, 2025
@jade-guiton-dd @mx-psi @mowies
[chore] Add CI check to enforce merge freeze during release (#37097)
b8bba71 
#### 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
@jade-guiton-dd @mx-psi
@mowies @chengchuanpeng
[chore] Add CI check to enforce merge freeze during release (open-tel…
8daadbd 
…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
Assignees
No one assigned
Labels
discussion-needed Community discussion needed release:blocker The issue must be resolved before cutting the next release release:retro Issues discussed in a release retrospective
Projects
None yet
Milestone
No milestone
3 participants
@atoulme @mx-psi @jade-guiton-dd