Add permissions check to valid_owner #62
Conversation
Previously, the valid_owner check used the Repositories List Teams API endpoint to check if a team was valid. Due to an issue in the GitHub API, that endpoint does not return child teams that would inherit a parent team's permissions. This commit changes the initial check for the validity of a team to use the Teams List Teams API endpoint, which lists all teams in a org to check if the team exists at all. If that check fails, it will clearly state that the team does not exist. As a follow up to the initial valid team check, this commit checks the Teams IsTeamRepoBySlug endpoint, which returns an object containing the actual permissions for the team being checked on the repo. By adding this check, we can verify that a user can actually provide their review on PRs that made the CODEOWNERS line.
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
==========================================
- Coverage 50.13% 47.00% -3.14%
==========================================
Files 12 12
Lines 375 400 +25
==========================================
Hits 188 188
- Misses 183 208 +25
Partials 4 4
Continue to review full report at Codecov.
|
|
I'm not surprised to see that I should add more unit tests. I'll work on that when I have a moment. |
|
This is just what we need, thanks for fixing this! |
|
I don't see an easy way to increase coverage in this file. All of the active check code is uncovered as it hits GitHub APIs – I can't tell if they're mocked nor how, but when I gave it a trivial try by calling Check with This might explain the file being 50% coverage target. @mszostok what's the path forward here? This is a critical fix for teams using parent team hierarchies, and the code looks good to me! |
|
Hi @jspiro today I will take a look. Basically, implemented functionality is covered by the integration tests: https://github.com/mszostok/codeowners-validator/blob/master/docs/development.md#integration-tests but the test coverage is not included in the report (see: https://github.com/mszostok/codeowners-validator/issues). Unfortunately, integration tests cannot be executed on PRs from external contributors because of the problem with tokens. I will run them manually. Btw. I see that the job failed on code quality check, see: https://travis-ci.com/github/mszostok/codeowners-validator/jobs/471190231 - it is easy to fix :) |
|
@mszostok Thanks for taking a look! If there's anything I can do to help out, please ask. |
There was a problem hiding this comment.
Once again thanks for your contribution!
I executed integration tests, and it works properly as I also simulated that some teams have only "read" permission, and they were detected properly 👍
I left a few comments which are minor. After resolving them we can merge the pull-request 🚀
If you do not have time for that I can take care of it as they are just small adjustments - if necessary ping me under this pull request 🙂
| } | ||
|
|
||
| // repo contains the permissions for the team slug given | ||
| repo, _, err := v.ghClient.Teams.IsTeamRepoBySlug(ctx, v.orgName, team, org, v.orgRepoName) |
There was a problem hiding this comment.
Thanks for adding that functionality, it was also requested some time ago (#21)
Unfortunately, this approach increases the number of external call to GitHub, probably in the near feature it will be good to switch to GraphQL which will remove problem with over fetched data, and we will be able to merge the ListTeams and IsTeamRepoBySlug into single query, see: #21 (comment)
but this is for future, for now, it lgtm 👍
There was a problem hiding this comment.
That is quite true. There are also potentially issues with GraphQL, which we've experienced with another project we contribute to, the GitHub Terraform provider – some calls because extremely, extremely slow in larger orgs (branch protection) and it annihilated the performance compared to REST. I doubt it'd be an issue here, just sharing our experience.
|
@mszostok As you indicated, Travis is hitting API limits, presumably due to unauthenticated requests having a limit of 60 per hour per IP. I suppose this is a good reason for GQL ;-) Have you considered porting this to GitHub Actions? I suspect that would paper over this issue, at least. |
|
If this is acceptable and gets merged, please remember to tag it :-) |
Yes, this is on my TODO list, as for all other project I already use the GitHub Actions. Maybe during this weekend I will have some time to do it and add also release pipeline cause right now it is pain in the ass as it is manual 😄 |
|
|
Looking forward to rolling this out on all our repos! Thanks for putting the project out there!
Description
Changes proposed in this pull request:
I haven't contributed to many Go Projects, so hopefully I'm following good conventions.
Related issue(s)
Fixes #40