Skip to content

Conversation

@gonfunko
Copy link
Contributor

The basics

This PR adds the pull-requests: write permission to assign_reviewers.yml. The docs say it needs this or issues: write, but I'm suspicious it may need both.

@gonfunko gonfunko requested a review from a team as a code owner March 19, 2025 17:23
@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Mar 19, 2025
@gonfunko gonfunko closed this Mar 19, 2025
@gonfunko gonfunko requested review from a team and RoboErikG and removed request for a team and RoboErikG March 19, 2025 19:30
@gonfunko gonfunko reopened this Mar 19, 2025
@gonfunko gonfunko closed this Mar 19, 2025
@gonfunko gonfunko reopened this Mar 19, 2025
@gonfunko gonfunko marked this pull request as draft March 19, 2025 19:46
@gonfunko gonfunko closed this Mar 19, 2025
@gonfunko gonfunko reopened this Mar 19, 2025
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Please fix.

@gonfunko gonfunko marked this pull request as ready for review March 19, 2025 20:31
@gonfunko gonfunko closed this Mar 19, 2025
@gonfunko gonfunko requested review from a team and BenHenning and removed request for a team March 19, 2025 20:32
@gonfunko gonfunko reopened this Mar 19, 2025
@BenHenning
Copy link
Collaborator

I'm assuming you've already seen this: https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-pull-requests.

https://docs.github.com/en/rest/issues/assignees?apiVersion=2022-11-28#add-assignees-to-an-issue suggests that the current permissions should work.

I'm having some difficulty backing this up, but can you try moving the permissions to be top-level instead of per-job? It shouldn't make a difference, but it would be nice to double check.

@gonfunko gonfunko requested review from a team and removed request for a team and BenHenning March 19, 2025 20:57
@gonfunko gonfunko requested a review from RoboErikG March 19, 2025 20:58
@gonfunko gonfunko requested review from a team and BenHenning and removed request for a team and RoboErikG March 19, 2025 21:22
@BenHenning
Copy link
Collaborator

BenHenning commented Apr 23, 2025

I've tested this in https://github.com/BenHenning/blockly/blob/develop2/.github/workflows/assign_reviewers.yml using BenHenning#1. You should be able to use pull_request_target with just pull-requests: write. The documentation is wrong in that it says issues OR pull-requests is needed. One is needed but it depends on the context.

Because of the nature of pull_request_target this won't actually pass until it's merged into develop (as that permission forces using the merged-in version even if you change it here per security reasons) which is why I tested against a base branch instead of within the PR.

Edit: I also have been trying to investigate the conventional commits failure, but that one is a tougher nut to crack. It definitely feels like it's intermittent as I sometimes see it pass, so it's tricky.

Copy link
Collaborator

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @gonfunko! From my testing, this should fix the check once merged in. :)

@gonfunko gonfunko merged commit 0a95939 into RaspberryPiFoundation:develop Apr 23, 2025
7 checks passed
@gonfunko gonfunko deleted the workflows3 branch April 23, 2025 17:46
@BenHenning
Copy link
Collaborator

Also: this will need to be done on the v12 branch, too, if we want CI to not fail there.

BenHenning pushed a commit to BenHenning/blockly that referenced this pull request Apr 30, 2025
* chore: Try another workflow permission.

* chore: Explicitly specify the GitHub token.

* chore: Try with contents: write.

* chore: Try write-all at the top level.

* chore: try regular pull_request.

* chore: Fix assign reviewers action configuration.
BenHenning added a commit that referenced this pull request Apr 30, 2025
_This is a cherry-pick of #8811 into the v12 release branch._

## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

(ish--largely relying on the original testing for this PR since it's actions-related)

## The details
### Resolves

This fixes the same issue as #8811 but on the v12 branch.

### Proposed Changes

See #8811.

### Reason for Changes

The failing assignee workflow will continue on v12 so the cherry-pick makes the next couple of PRs for this branch a bit nicer, but it's not a strong must-have since we'll eventually merge `develop` into `v12` which would then include #8811.

### Test Coverage

N/A -- There's no strong benefit from automated tests for this workflow, and it was manually tested as part of #8811 (see #8811 (comment)).

### Documentation

N/A -- No documentation changes are needed for this.

### Additional Information

None.
BenHenning added a commit that referenced this pull request May 2, 2025
## The basics

- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)

## The details
### Resolves

This fixes the ongoing CI failure for the conventional auto-labeling.

### Proposed Changes

This fixes the permissions in a way that should work. It may be the case that 'issues' only needs to be 'read' but I'm basically just copying what's done in https://github.com/GoogleCloudPlatform/developer-journey-app/blob/main/.github/workflows/auto-label.yml since it's working for them.

### Reason for Changes

We want this workflow working--it's preferable to avoid getting used to a failing CI workflow (ideally every PR has zero CI failures).

As for the specific changes, note that the check will still fail in this PR. Similar to #8811, it's not expected that the CI workflow will pass in this PR until the change is checked in since the workflow uses 'pull_request_target'. While I haven't verified this change directly, I'm fairly confident it will work given the project linked above and our successes with fixing the auto assigner workflow.

Finally, the 'contents: read' bit is unnecessary since that's the default permission for `GITHUB_TOKEN` per https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#permissions-for-the-github_token.

Edit: It seems that the check actually is passing with these changes--that's a bit surprising to me.

### Test Coverage

N/A

### Documentation

N/A

### Additional Information

None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: chore General chores (dependencies, typos, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants