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

New commands for user invitation #27

Merged
merged 10 commits into from
Mar 27, 2017
Merged

New commands for user invitation #27

merged 10 commits into from
Mar 27, 2017

Conversation

jaybrueder
Copy link

Hello,

I implemented Hubcommander for a customer of mine. We soon realised that some important commands are missing for us.

I have added these commands to the code and thought you guys probably like them too.

!InviteUser
A command that adds a GitHub user to your org. You can also set the permission level (admin or member)

!AddUserToTeam
A command that adds an existing user in your org to a specific team. If the user is not yet a part of your org, he will also get an invite. I had to update the existing invite_user_to_gh_org_team method to be able to also set a role here.

I am happy to get your feedback and commit any changes this might still need.

There will probably be coming some more updates by me in the futute. The customer really digs Hubcommander :)

@mikegrima
Copy link
Contributor

Hello @jaybrueder ! Thank you for the PR.

I will try to review this later this week. But to provide some general feedback regarding the commands:

For user invitation, at Netflix, we have a custom HubCommander plugin for this capability, as we want to link a user's (employee in our case) GitHub ID with our internal employee tracking system. This allows us to audit who is actually using our GitHub organizations, and also handle removals from the org when they leave the company. This is something that you should keep in mind, as inviting users to an org through this method could cause user management headaches in the future.

@mikegrima mikegrima self-requested a review March 23, 2017 00:57
Copy link
Contributor

@mikegrima mikegrima left a comment

Choose a reason for hiding this comment

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

Please review these changes. PR looks good. Once these are addressed, I will merge it in.

github/plugin.py Outdated

data = {"role": role}

# Add the outside collab to the repo:
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment.

github/plugin.py Outdated
'Accept': GITHUB_VERSION
}

# Add the outside collab to the repo:
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment.

github/plugin.py Outdated
response = requests.get('{}{}'.format(GITHUB_URL, api_part), headers=headers, timeout=10)

if response.status_code != 200:
raise ValueError("GitHub Problem: Could not list teams: {}".format(response.status_code))
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to: "GitHub Problem: Could not list teams -- received error code: {}".format(response.status_code).

github/plugin.py Outdated
@@ -82,8 +82,23 @@ def __init__(self):
"user_data_required": True,
"help": "Delete a GitHub repository.",
"enabled": True # It is HIGHLY recommended you have auth enabled for this!!
},
"!InviteUser": {
Copy link
Contributor

@mikegrima mikegrima Mar 26, 2017

Choose a reason for hiding this comment

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

We should remove this -- or rather, make it a shortcut to !AddUserToTeam.

I feel that it is good practice to have all members exist within a team. Perhaps this can be modified where you can configure a default_team to add new members to.

Copy link
Author

Choose a reason for hiding this comment

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

I agree on removing it. After having it running with the two commands for a week now, I can see that this command is never used as the desired behaviour is actually the AddUserToTeam command.

github/plugin.py Outdated
"func": self.add_user_to_team_command,
"user_data_required": True,
"help": "Adds a GitHub user to a specific team inside the organization.",
"permitted_permissions": ["maintainer", "member"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Change name to permitted_roles to be consistent with GitHub verbiage. Also, member should be the default option and perhaps the role made to be optional. We would have to test if optional parameters can be provided.

@jaybrueder
Copy link
Author

@mikegrima I made the requested changes. Please let me know if I can improve anything else :)

@mikegrima
Copy link
Contributor

LGTM

@mikegrima mikegrima merged commit 3730cc9 into Netflix:develop Mar 27, 2017
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.

2 participants