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

Avoid initiating CI runs for members who are not in the mantid-developers GitHub team #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

warunawickramasingha
Copy link

@warunawickramasingha warunawickramasingha commented Nov 8, 2024

A validation was added to avoid initiating CI runs for the Pull requests from the external parties who are not members of the mantid-developers GitHub team. The notion behind making this change is to increase the security of the CI environments to avoid any malicious PRs.

The GitHub API used: https://docs.github.com/en/rest/teams/members?apiVersion=2022-11-28#get-team-membership-for-a-user

@SilkeSchomann SilkeSchomann self-assigned this Nov 11, 2024
Copy link

@SilkeSchomann SilkeSchomann left a comment

Choose a reason for hiding this comment

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

I have reviewed the code and compared the commands with the GitHub API and similar examples for leeroy. Looks good to me!

Copy link
Member

@martyngigg martyngigg left a comment

Choose a reason for hiding this comment

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

I don't know how much work this is but is it worth writing a comment back to the PR to say something like:

"The CI workflow has not been triggered as %author is not an approved user. Please contact the mantid development team for more information".

This would let be an indication that something has happened and new users know what to do.

@@ -209,6 +215,21 @@ retry:
return
}

func isValidMember(author string) bool {
orgName := "mantidproject"
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth putting these values; orgName, teamName & author in the config.json? That way all of the configuration values are in one place.

Maybe make this a list and include mantid-contributors? This list includes external collaborators but they are trusted people too.

Choose a reason for hiding this comment

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

Thanks Martyn for the review!
Yes it is a good idea to make teamNames a list and to load orgName as well from config.json rather than hardcoding here.

Since, author is the original user who has made the PR, can we define its value in the config.json?
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

No you're correct, author must of course come from the PR itself 🤦🏻. I got carried away typing the list!

Copy link
Member

Choose a reason for hiding this comment

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

For who gets to kick off builds, definitely those in mantid-developers (which is nested) and mantid-contributors is reasonable. I wouldn't add any other groups. For example, gatekeepers should be in mantid-developers as well.

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.

4 participants