-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Adds check to ensure org doesn't have any outside collaborator #541
feat: Adds check to ensure org doesn't have any outside collaborator #541
Conversation
2eca9f9
to
48fe503
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting the org_checks in the repo_checks.py
executable may not be the best choice. It leads to a couple of different issues:
- The name of the script is now misleading which can lead to future confusion.
- We're losing out on some of the mechanisms we've built to control which checks get run in the current iteration. eg. We have no way to say don't run the org checks or just run a specific org check. We could fix this in place but I think that's going to make the code messier and harder to read.
A note on outside collaborators;
- Orgs can have outside collaborators, but repos can also have outside collaborators
which are not listed at the org level. So you'll need a repo check for collaborators...Thinking a bit more about it, I think you don't need to have an org level check for this because that's pretty easy for us to review on the org itself. So maybe just focus on the repo level check?
Other notes as I'm reviewing this:
- I think it's useful pull things out into functions when things become re-usable but otherwise jumping to a function just to continue reading the code and then jumping back is not valuable in my mind. I tend to not have too many functions unless there is a clear reason to split out the code to make it easier to test or to re-use a bit of code multiple times. This is my opinion so not strictly a request to remove all functions you've split out
main()
but to re-assess what their usefulness is and if we can achieve some of the same readability by adding comments instead.
48fe503
to
f965b29
Compare
@feanil Thanks for flagging the concern I initially thought api is not returning the list of outside collaborators as no relevant data is available in the response but we can add filter. So task is done in a simple way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
96d2efd
to
8441baf
Compare
8441baf
to
adbfd24
Compare
Adds check to ensure org doesn't have any outside collaborator
Ticket: openedx/public-engineering#169
Git api used to add check:
https://docs.github.com/en/rest/orgs/outside-collaborators?apiVersion=2022-11-28
Use following command to test: