Skip to content
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

Merged

Conversation

farhan
Copy link
Contributor

@farhan farhan commented Jul 23, 2024

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:

repo_checks \
--org $ORGANIZATION \
--repo $REPO

@feanil feanil force-pushed the farhan/ensure-no-outside-collaborator branch from 2eca9f9 to 48fe503 Compare July 24, 2024 14:16
@farhan farhan requested review from salman2013 and irtazaakram July 24, 2024 14:42
Copy link
Contributor

@feanil feanil left a 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;

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.

@farhan farhan force-pushed the farhan/ensure-no-outside-collaborator branch from 48fe503 to f965b29 Compare July 26, 2024 12:52
@farhan
Copy link
Contributor Author

farhan commented Jul 26, 2024

@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.

@farhan farhan requested a review from feanil July 26, 2024 12:56
@farhan farhan linked an issue Jul 30, 2024 that may be closed by this pull request
1 task
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@farhan farhan force-pushed the farhan/ensure-no-outside-collaborator branch 2 times, most recently from 96d2efd to 8441baf Compare August 8, 2024 14:17
@farhan farhan force-pushed the farhan/ensure-no-outside-collaborator branch from 8441baf to adbfd24 Compare August 8, 2024 14:19
@farhan farhan merged commit dd51bd3 into openedx:master Aug 8, 2024
3 checks passed
@farhan farhan deleted the farhan/ensure-no-outside-collaborator branch August 8, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repo Check: No Outside Collaborators
2 participants