Adding more checks for team permissions. Adding more ENV variables #4
Conversation
There was a problem hiding this comment.
Hi @njegosrailic, thanks for PR!
Review
-
Adding environment variables to be able to run file check and code owners permissions check separately.
It's a really nice thing. But IMO it will be better to do it in the same fashion as it was implemented e.g. in GolangCI-Lint. Having a separate env variable for each check with true/false is a little overhead. Better will be to have just the
CHECKSenv variable. If empty then all check enabled. If the user specifies sth, e.g.CHECKS=owners,paththen only those two checks are enabled, here is the example how it can be handled inmain.go: https://github.com/mszostok/codeowners-validator/compare/check-enabled-disabled?expand=1
FYI
-
Also we are hitting API rate limits.
We are using the GitHub API Cache: https://github.com/kubernetes/test-infra/blob/master/prow/scaling.md#github-api-cache. If you have a big infrastructure the is good to have that. But I will definitely add the internal cache to the
odeowners-validator. Probably I will go with a similar caching mechanism as Kubernetes implement inkubectl. But currently I'm not able to tell when it will be done, here I have a draft PR: Add HTTP response disk cache #7
After merging your pull request I will also merge this one: #5, where I've added Duplicated Pattern Checker - you are also welcomed to do the review if you want :). After that, I will create a new release :)
|
@njegosrailic I will also try to check the GitHub GraphQL API and maybe I will be able to replace the rest calls with graphql queries, as a result I will be able to reduce the problems with hitting API GitHub quota. btw. If you do not have time then we can merge your pull request and I will adjust the selective testing in this pr: #8 |
|
Hey @mszostok , Thank you very much for all the information you provided and suggestions how to improve the code. :) I'll update my PR soon. My apologies but I'm very busy these days. Also, we are going to try: https://github.com/mszostok/codeowners-validator/compare/check-enabled-disabled?expand=1 for other services not primary for code owners. I have checked with github support and they have no any official tool to validate code owners. |
yep, this is true and this was also the reason that I created such a tool. In the upcoming week, I want to do the release and use that officially in Kyma project created by SAP. so new improvements will come soon
no problem :) |
njegosrailic
force-pushed
the
nrailic_add_more_envs
branch
from
fa80c10 to
6b145d6
Compare
|
FYI: CI checks failed because the imports are not sorted You can execute those checks also locally: In few days I will update documentation and add developer guide:) |
njegosrailic
force-pushed
the
nrailic_add_more_envs
branch
from
4fbfb2b to
d75ed99
Compare
Actually I accidentally dropped an line. It should be all set. The problem is because I have opened the PR from my forked repo and I did some changes there to be able to compile it. Once you merge the change will drop my fork and migrate config to yours. |
|
@njegosrailic and what about your feature with checking the user permission? Right now it’s not included in this pr You want to submit it in separate pr? |
Added that back. |
|
Thanks @njegosrailic for your PR! The new release with your change is already available on the GitHub: https://github.com/mszostok/codeowners-validator/releases/tag/v0.2.0 Check the release notes for more detailed info 🙂 |
According to https://github.saobby.my.eu.orgmunity/t5/How-to-use-Git-and-GitHub/CODEOWNERS-works-with-users-but-not-teams/td-p/4986 a team needs write access to a repository.