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

[POC] liberalize access to inviting people to the org #29

Closed
wants to merge 1 commit into from
Closed

[POC] liberalize access to inviting people to the org #29

wants to merge 1 commit into from

Conversation

williamkapke
Copy link
Contributor

@williamkapke williamkapke commented Apr 15, 2016

Here is an option for @mikeal's request to liberalize access to inviting people to the org.

I created a video to show it in action:
https://www.youtube.com/watch?v=3tr4yTtHQ_g

How it works

Modify a repo's README.md

Place a special <!-- team:name --> comment in the repo's readme.md:

## Members
<!-- team:website - all @mentions in this section will be added to the team by the bot -->
- @williamkapke
- @Fishrock123

<!-- team -->

NOTE: You must specify the team's name in the opening comment like shown above!

Create/update a PR

When a PR is submitted or changes are pushed, the bot will evaluate the commit(s) for changes to the README.md file. If changed, it will extract the content within the <!-- team --> comments find all @mentions and do a diff with the list of current team members.

Accept the PR

Accepting a PR sends a push event. So, any push- even ones outside the PR process, will get picked up by the bot.

When the bot receives the push event, it will do the same parsing + evaluating as when the PR was created/updated, but instead of creating a comment- it applies the additions AND removals to the team (if any).

To be very clear...
This means anyone with commit rights can cause people to be added or removed from the team. It should also be noted that it does not validate that the repo is owned by the team being changed. In other words, someone could change the team name to cause addition/removals to any team in the org. Validation could be added if this is a concern.

Assign it to the bot!

As somewhat a proof-of-concept, I added the ability to assign the PR to the bot to cause it to re-evaluate the PR. The original comment will be updated (plus an additional timestamp) with the results.

The bot will unassign itself when finished.

@phillipj
Copy link
Member

This looks pretty slick 👍 In the video you assigned the bot to the PR, is that required for this to work?

This means anyone with commit rights can cause people to be added or removed from the team.

Could we check if the committer is an owner of the repo in question?

@Fishrock123
Copy link
Contributor

can we please use only characters that can be typed on a regular keyboard? Math symbols are obscure, awkward, and raise the barrier for contributions. :S

@williamkapke
Copy link
Contributor Author

you assigned the bot to the PR, is that required for this to work?

Nope- just added that to show that it can be done & to show the update

typed on a regular keyboard

It's ALT+f... It can be changed if we must. ;)

I'm certain there are many things to change about the code. I want to wait to see if this is something we would actually move forward with before committing more time to it. Sound fair?

@williamkapke
Copy link
Contributor Author

@phillipj

Could we check if the committer is an owner of the repo in question?

Owner? Team maintainer? Eitherway- yes, we can. However, consider this case: if someone just has commit rights, they can get the changes live in the README and the bot would ignore them. Then the README and the members will be out of sync. The next time someone does a PR to the readme- it will show changes they didn't do. Basically- it causes trouble.

@phillipj
Copy link
Member

@williamkapke good point.

Thinking out loud to get some thoughts going here:

Add committers by mentioning the bot in a comment

@nodejs-github-bot add committers @williamkapke @phillipj

As long as the commenting user as an owner/team maintainer could trigger the bot to add the given users to the current repo. Maybe too simple though.

Create issues in a dedicated repo for adding committers

We could create a new repo somewhere dedicated to the bot adding committers to any repo in the nodejs org. The issue would have to specify which users should be added into which repo. Again owner/team maintainer check should be in place.

Any thoughts?

@Fishrock123
Copy link
Contributor

As long as the commenting user as an owner/team maintainer could trigger the bot to add the given users to the current repo. Maybe too simple though.

All it needs to do is invite the user to the org. The team maintainer bit is good enough to do the rest from the UI.

@williamkapke
Copy link
Contributor Author

Part of this solution I was going for was keeping things documented and transparent. No one made this a requirement- it is something I personally would like to see. So, I chose the README since it is where most repos seem to have the list- but also because it is right out front and not lost in an issue thread. There may be some cases where people don't want to be listed- but I don't know them.

I went with the PR approach because I felt it gave teams that workflow of propose-discuss-(maybe vote)-accept.

For committers, this PR could easily be extended to have a <!-- committers --> section too.

@phillipj
Copy link
Member

@williamkapke although I like the direction you've taken here, in my experience it's (understandably) a lot easier to get a "go ahead" for small things, and improve from there. That said, this might be too much to start with.

From what I've heard and @Fishrock123's comment, it seems the most urgent need is for team maintainers to invite future collaborators into the org. Could inviting users to the org be the first iteration of this, and stash the rest of the changes for later?

@williamkapke
Copy link
Contributor Author

@mikeal can you take a moment to review and weight in? Since it was his feature request, I'm hoping he can also play Product Manager on this one ;)

@Fishrock123
Copy link
Contributor

I still think it would be more reasonable t have this run on it's own (and have minimal changes). (Security etc)

As a note, it should probably check for team maintainers of teams as verification.

@phillipj
Copy link
Member

phillipj commented Jun 6, 2016

@mikeal ping - see @williamkapke question above in #29 (comment)

@mikeal
Copy link

mikeal commented Jun 6, 2016

I LIKE!

@mikeal
Copy link

mikeal commented Jun 6, 2016

Oh, when it invites them to the org it should also add them to the "members" team.

@MylesBorins
Copy link
Contributor

MylesBorins commented Jun 6, 2016 via email

@jbergstroem
Copy link
Member

@thealphanerd if "done right", a commit/issue/pr thing should probably exist? We just did a similar thing in the build group where we voted as part of an wg meeting.

@williamkapke
Copy link
Contributor Author

@thealphanerd @jbergstroem actually, part of my plan for automating WG Meetings is to take the MD from the google docs and move it to a MD file that is PRd to the WGs repo when the meeting issue is closed.

All of this (of course) would be opt-in per WG though since they have full autonomy. :)

@phillipj
Copy link
Member

@williamkapke seeing your comment in the build WG meeting nodejs/build#438 (comment), is this or anything else dependant on where/how the bot is deployed?

@williamkapke
Copy link
Contributor Author

@phillipj
Where: It will run fine an any service- no special needs for that.
How: It is just another script that reacts to events. The only difference from any of the other scripts ya'll have been cranking out is the elevated permissions I mentioned in nodejs/build#404 (comment). Well, actually, THIS specific script only needs the elevated permission for adding people to the org. (not Google & YouTube)

...in other words, this script requires process.env.GITHUB_TOKEN to have read/write access to admin:org:
image

Also note that THIS script (while fully functional) was a proof of concept. I anticipate that we may take steps to isolate this script with elevated permissions... and open the flood gates on "nits" that should be changed ;)

@phillipj
Copy link
Member

@Fishrock123 you had some thoughts about creating yet another bot to provide org admin access to, care to elaborate on that? What would make that safer than giving the existing bot those rights? Should that bot be closed source?

@mhdawson
Copy link
Member

@williamkapke just an FYI if you need something to read/write google docs: https://github.com/mhdawson/google-drive-wrapper

outsideris added a commit to outsideris/nodejs-ko that referenced this pull request Dec 2, 2016
to mangage for nodejs-ko members
These members contributed in last 1 year.
It will update nodejs-ko members in github.
See detail: nodejs/github-bot#29 (comment)
@outsideris
Copy link

Please ping me once you set up the bot. @williamkapke

@Fishrock123 Fishrock123 force-pushed the master branch 2 times, most recently from 5b9d4ef to b2a9558 Compare December 8, 2016 19:08
@williamkapke williamkapke changed the title liberalize access to inviting people to the org [POC] liberalize access to inviting people to the org Dec 31, 2016
@williamkapke
Copy link
Contributor Author

See #114

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.

8 participants