Update PR labels after review

[jakirkham]

When a reviewer performs a review ( #20860 (review) ), it would be good if the bot could relabel the PR accordingly ( #20860 (comment) ).

Thought maybe this was implemented before (and so perhaps there is a bug), but may be misremembering.

[jakirkham]

cc @BastianZim (in case you have thoughts 🙂)

[BastianZim]

Thanks for adding the label there. 😄

The problem is that the GitHub Actions permissions currently don't allow that. It is impossible to check if a person interacting with the PR is part of the review team or not. This is because the Action is run from a fork and has very few permissions, something that is set by GitHub and cannot be changed.
It is also impossible to have the action check if the person is in a list, but I don't remember anymore why exactly. The only thing I can remember is that GitHub also didn't give the action the necessary rights.

So, in short, can't be done because the action is run from a fork and does not have sufficient permissions.

cc @carterbox

[jakirkham]

Do we need to check the team of the reviewer? Or could we just always do this when there is a review?

[carterbox]

The detailed explanation of the limitations is in Bastian's original issue proposing automated labels.

#18023 (comment)
#18023 (comment)

Do we need to check the team of the reviewer? Or could we just always do this when there is a review?

This may be possible. I don't exactly remember what the bot is allowed/not allowed to do in each of fork and main contexts. I think we didn't consider this approach because people who do not have merge permission often will review PRs; if we don't check membership, then this will dismiss the tags prematurely.

[jakirkham]

FWIW think that is ok. We welcome folks here to help out and review. Over time we increasingly trust them (based on their track record) and start handing over permissions. Thus it seems reasonable for the labels to take their review into account. We can always override and relabel as needed.

Should add am just thinking about how we can improve this process (that y'all put into place). Though feel free to push back if you disagree 🙂

[BastianZim]

This has been some time ago now so take it with a grain of salt, but I think there was just a permission problem where we couldn't extract some of the info required and couldn't run the action because it is on a fork – could be though that I'm misremembering this and it is possible, not sure.

Though feel free to push back if you disagree 🙂

Not at all – fully agree that this would be great to have and is also what we initially tried to implement. The problem is just that we weren't able to, from a technical point of view. :)

[carterbox]

Yeah... I also am not sure that a bot triggered in the review context has permission to manage labels on PRs. I feel like we tried, but it didn't. Someone who is curious can look back through the git history to see whether that approach is there or not.

[manics]

Possible alternative to workaround permissions problems: on.schedule.cron[] hourly job that fetches PRs updated in the last 2 hours (is:pr is:open sort:updated-desc updated:>YYYY-MM-DDTHH:MM seems to work in search), and updates the labels? GitHub recently added fine grained permissions for PAT tokens which reduces the risks of using a PAT.

[BastianZim]

Yes, a cron job run on main is probably the best solution. I think we were really limited in GitHub Actions time, so didn't go with that. But yes, would be the easiest solution.

[manics]

If there's limited GitHub action minutes available I think the job could be run in a new GitHub bot account, as long as it's a collaborator on this repo and the PAT has sufficient privileges.

[carterbox]

I'm not worried about action minutes. We could scale back to once a day. That would be less often than what the current workflow is triggered.

[carterbox]

Possible alternative to workaround permissions problems: on.schedule.cron[] hourly job that fetches PRs updated in the last 2 hours (is:pr is:open sort:updated-desc updated:>YYYY-MM-DDTHH:MM seems to work in search), and updates the labels? GitHub recently added fine grained permissions for PAT tokens which reduces the risks of using a PAT.

I was poking around the docs. I think we want to filter specifically to PRs that have changed review status to changes_requested after some date.

is:open type:pr review:changes_requested updated:>2022-11-19

[BastianZim]

Found some more info here #18023 (comment) I think we had primarily permission issues so we can definitely only run this based on a cron job on main, I guess.

[jakirkham]

Perhaps we could start a GH Discussion asking for the particular permissions (or other changes) we would need for this use case. Happy to put this in front of folks we know at GitHub once opened

[BastianZim]

Yeah I guess we need to have a look at this again because the labelling is starting to fail: https://github.com/conda-forge/staged-recipes/actions

I'll see if I can test this again over the weekend.

What should be the trigger to switch from review-requested to awaiting-author, in an ideal situation?

[carterbox]

What should be the trigger to switch from review-requested to awaiting-author, in an ideal situation?

Review submitted by anyone?

[BastianZim]

Ok, the bot is failing more or less completely now. Not sure why.

[carterbox]

Maybe GitHub changed the permissions available to our trigger? But the GITHUB_TOKEN Permissions section of the job setup says "Issues: write".

[carterbox]

I think they separated issue/pr permissions, so I added pr write permissions to the workflow. Seems to be working now?

[BastianZim]

Shouldn't we have seen a warning before that then?

Generally, you can add the following to the yaml but I'm not sure if this is giving too many permissions...

permissions:
  id-token: write
  contents: read

[BastianZim]

I think they separated issue/pr permissions, so I added pr write permissions to the workflow. Seems to be working now?

Makes sense. I'm just worried that that's too many permissions. But yeah, it's fixed for now.

[carterbox]

AFAIK, the permissions don't get any more granular without creating a special access token. Could also set permissions for each job, but I think they would all need the same permission, so that's moot.

[jakirkham]

Shouldn't we have seen a warning before that then?

Generally, you can add the following to the yaml but I'm not sure if this is giving too many permissions...

permissions:
  id-token: write
  contents: read

Edit: Note the actual change used is here ( ec9cc75 ), which looks like this

   permissions:
     issues: write # for adding label to an issue
     pull-requests: write # for adding label to a pr

cc @conda-forge/core (in case others have thoughts on this)

[beckermr]

Is not worry too much here on the permissions for this token. Happy to share more thoughts offline.

[beckermr]

FYI, we've decided to remove the token that this job was using. We won't be putting it back and so we'll have to find another way to do the labels.

[carterbox]

With no token, we may be able to switch to a cron job trigger instead of reacting to activity on the PR/Issue. I recall that default permissions are greater for non-pr triggers, but I'm not sure if cron trigger has the specific permissions we need for this task. We'll need to test.

If not, is setting up a bot with a token an option? Otherwise, this automation is dead.

[carterbox]

Actually, I think the only part that needs a special token is team membership checking, which I think we already discussed is not necessary, but just nice to have.

[beckermr]

We looked at the pr event that triggers on main right? I think the issue was getting the members of some teams?

[carterbox]

I rechecked, and the pull_request_review trigger only has read permissions for PRs from forks, so the bot triggered by reviews cannot manage labels without a special token. That's why we currently use pull_request_target to listen for labelling events.

Thus, cron seems to be the best option to respond to reviews.

[jakirkham]

Wonder if there is a better option than cron job. A lot of early scripts in conda-forge were cron job based, but we needed to move them to event based due to performance and reliability issues encountered due to scaling. Maybe that isn't an issue at staged-recipes, but still it does worry me a bit.

Just to try a different idea out, what if this labeling happened somewhere else where we were ok having the token? Namely we could subscribe to events on this repo and when certain events occurred, make the appropriate change. Thoughts?

[carterbox]

An external bot with correct token could do this job, but whether or not such a bot is allowed to do this job is up to @beckermr (?).

[jakirkham]

The linter is such a bot that comments on PRs

It seems reasonable that we could have a labeling bot. We could restrict it to staged-recipes if we wanted. Though maybe labeling could be helpful in feedstocks too?