Auto add CICD label when PR author is a member or owner of flutter organization#4997
Auto add CICD label when PR author is a member or owner of flutter organization#4997gmackall wants to merge 13 commits intoflutter:mainfrom
Conversation
| /// `OWNER` indicate the PR author belongs to the organization. | ||
| static bool _isOrgMember(PullRequest pr) { | ||
| final association = pr.authorAssociation; | ||
| return association == 'MEMBER' || association == 'OWNER'; |
There was a problem hiding this comment.
Are any of these valid:
COLLABORATOR, CONTRIBUTOR, FIRST_TIMER, FIRST_TIME_CONTRIBUTOR, MANNEQUIN, MEMBER, NONE, OWNER
I'm not sure what COLLABORATOR vs CONTRIBUTOR
There was a problem hiding this comment.
I think we want MEMBER or OWNER only here, as that maps to the flutter hackers group, as I understand
|
I can get some |
| await _addCICDForRollers(pullRequestEvent); | ||
| if (pullRequestEvent.changes != null && | ||
| pullRequestEvent.changes!.base != null) { | ||
| await _addCICDForRollersAndMembers(pullRequestEvent); |
There was a problem hiding this comment.
This is a slight behavior change, I made it this way because before the path for scheduling tests checked these conditions:
363e9cd#diff-4d5f5e21f5b7eda8e7775b9a678fb60bb6cfba3321799948fb2b9a55cefbe439L202-L206
And I figured it was also fine to apply those checks for the roller case. Let me know if not
|
I checked the logs for the last 30 days matching the exact string:
I think we should limit this to just MEMBER. I'm assuming @Piinks can confirm that "MEMBER" is the correct 'person with write access'? |
Yeah I figure if OWNER does matter it will matter for probably only 1 person. But I'm fine removing |
|
I am not sure that MEMBER == folks with write access. There are Flutter GitHub org members without write access |
|
Yeah, AFAICT, there are 426 people that are org members, and only 289 are flutter-hackers (our current equivalent to write access) |
Hmm can all of those people add labels to issues? Or are labels restricted by write access? |
|
Ok, if that's the case, then we still need to build the "members with write access" list that I talked about before. |
Currently, only folks with write access can apply labels. 'Triage' access also allows people to apply labels, but we do not use the triage role currently. |
|
When you say our "current equivalent to write access", that is mechanically enforced? I.e. there are members of https://github.com/orgs/flutter/people?query=role%3Aowner, which, if they tried, could not add labels? I'm sort of confused how write access is conferred, is it somehow synched with an external list, or is it tied to a github property we can query? |
|
It is enforced by Github, not anything bespoke. Owners can apply labels. We don't assign roles individually, we manage them through Github teams. Owner is a special case, it is separate from the read|triage|write|maintain|admin roles Github provides for permissions management.
We manage the 'write' role through the flutter-hackers Github team, which I think should be something that can be queried? |
It looks like we currently have logic already doing this for determining if the pr is actually approved I will just use that logic here instead of the current author association check i have |
| /// | ||
| /// Note that we catch here as the api returns a 404 if the user has no | ||
| /// membership in general or is not a member of the team. | ||
| Future<bool> isTeamMember(String team, String user, String org) async { |
There was a problem hiding this comment.
I copied this from
as there are duplicate
github_service.darts and this one did not have the method. It may be good to de-dupe these at some point
| ) async { | ||
| final pr = pullRequestEvent.pullRequest!; | ||
| final slug = pr.base!.repo!.slug(); | ||
| final author = pr.user!.login!; |
There was a problem hiding this comment.
I double checked that this returns the author's user, and not the user of the person interacting with the PR:
https://pub.dev/documentation/github/latest/github/PullRequest/user.html
Fixes flutter/flutter#183618