-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add script for adding contributors #166
Conversation
I haven't tried running this but just in case it's slow to get the list of PRs and people, you should be able to use GraphQL to get this very quickly - see the first pr_consistency script for an example. That can get info for all PRs in tens of seconds. |
Add limit of number of repeated invites |
We do not want to be annoying, so only send new invitation if it has been long enough.
@eteq @hamogu @adrn -- this is ready for a look and merge if you think it looks good. Note that merging WILL NOT TRIGGER INVITES because the action runs the script with the Merging it will allow for further error checking. Let me know if you want it in a folder of its own so the top level isn't so cluttered. |
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.
Just a few minor suggestions, otherwise this looks good!
- uses: actions/checkout@v2 | ||
|
||
- uses: actions/setup-python@v2 |
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 just had dependabot update these in a bunch of my own repos, so might as well use the latest version?
- uses: actions/checkout@v2 | |
- uses: actions/setup-python@v2 | |
- uses: actions/checkout@v3 | |
- uses: actions/setup-python@v3 |
fail_mesage = ('Failed because previous invitation did not expire. ' | ||
'Instead, the previous invitation failed because ' | ||
f'{failed_invite["failed_reason"]}') | ||
fail_mesage |
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.
Did you want to print or raise an exception or return here?
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.
One minor change to suggest , but overall LGTM.
One thing that certainly doesn't need dealing with now but might once we go out of "dry-run" mode: it might be better if the package repos themselves could have the information on their policies, instead of it living in the json file here. That way the individual repo maintainers can keep track of it themselves with their own write powers instead of depending on PRs here.
BTW, I think this is relevant to: astropy/astropy-project#160 |
It probably requires similar token scope but is checking something a little different. Definitely could be part of a package of regularly-run jobs to handle these sorts of membership things. |
Co-authored-by: Adrian Price-Whelan <[email protected]>
Co-authored-by: Adrian Price-Whelan <[email protected]>
Co-authored-by: Adrian Price-Whelan <[email protected]>
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.
As discussed today (2022-04-07) during infrastructure tag-up (@eteq , @bsipocz , @saimn , @tomdonaldson, and myself), there needs to be a "paper trail" more permanent than GitHub Actions log. Perhaps when invitations are sent (or even follow-up actions), memo can be automatically posted over at Matrix (what Stuart would have wanted) or Slack. (I suggested GitHub issue but it was not accepted.)
Motivation of this "paper trail" is for accountability, in case something happens. Useful info would be "who got invited when for what" and so on.
Kudos on this PR to automate some of the policies!
Co-authored-by: P. L. Lim <[email protected]>
Re: #166 (review) - just to give @mwcraig added context, if you'd rather get the "dry run" mode going now we could do that without what @pllim is saying just to gather data to be sure its behaving right, but I agree that we should address the "paper trail" item before we actually turn it on. |
Co-authored-by: Erik Tollerud <[email protected]>
I had a couple of thoughts about that. Right now we have no paper trail at all of who has been invited -- someone emails the CoCo and we, without announcement, add the person. Suggestions (thinking out loud here):
I do think we could merge this as-is to see what happens |
Merging so we can see what happens....opening an issue for remaining items, including paper trail. |
The intent of this is to semi-automate adding people who have had PRs merged in coordinated packages (or astropy core) added to the astropy organization so that coordinated package maintainers can assign them to issues, etc.
To do before merging: