Added listing outside collaborators for an organization by glapul · Pull Request #538 · google/go-github

glapul · 2017-02-01T13:27:13Z

2fa_disabled filter is supported too.

I created a new file [orgs_outside_collaborators.go], but I'm not sure whether it wouldn't be better to rename [orgs_members.go] to something like [orgs_members_and_outside_collaborators.go] as it's not clear to me where would the function that converts a member to an outside collaborator fit best.

[orgs_outside_collaborators_test.go] contains tests that are basically recycled from [orgs_members_test.go].

I thought about adding an integration test, but I think that setting up a test scenario is either very hard or straight impossible right now (as we would have to be able to create an organisation and users, which I think is impossible without an Github Enterprise account).
So I fell back to testing it a bit by hand, and those tests passed.

By the way, it's not clear to me why only the organisation's owner can list the organisation's outside collaborators (that's the response API gave me when I tried listing google's). It seems that this is inferrable by just going through all the organisation's repositories, so being a member should suffice. Or am I missing something?

2fa_disabled filter is supported too.

gmlewis · 2017-02-01T19:14:51Z

Why not extend the already-existing Organizations.ListMembers method (in orgs_members.go) and make OutsideCollaborators bool a new option of ListMembersOptions ?

It currently already handles the PublicOnly option and seems like a good fit to me.

As an added bonus, it already has all the Filter and ListOptions ready-to-go.

dmitshur · 2017-02-01T22:03:12Z

@gmlewis, that's a good idea to consider.

However, I see that GitHub API really lists this as a separate endpoint. Compare:

Members list - https://developer.github.com/v3/orgs/members/#members-list
List outside collaborators - https://developer.github.com/v3/orgs/outside_collaborators/#list-outside-collaborators

Additionally, removing an outside collaborator is also different than removing a normal member:

Remove a member - https://developer.github.com/v3/orgs/members/#remove-a-member
Remove outside collaborator - https://developer.github.com/v3/orgs/outside_collaborators/#remove-outside-collaborator

Our current method for removing organization members does not take an options struct:

https://godoc.org/github.com/google/go-github/github#OrganizationsService.RemoveMember

So we would be forced to have a different method for removal (or add an options struct).

You can also "convert member to outside collaborator". It suggests to me these things are different enough that listing them via different endpoints makes sense.

While the idea of merging the listing endpoints makes sense now, I worry that future changes/additions to the API that further diverge outside collaborators and members may cause trouble for our API.

What do you think?

gmlewis · 2017-02-01T22:09:24Z

OK, I'm fine with keeping it separate then, @shurcooL.

gmlewis

Looking good, @glapul - thank you!
Just a few minor changes requested.

gmlewis · 2017-02-02T19:10:49Z

@@ -0,0 +1,51 @@
+// Copyright 2016 The go-github AUTHORS. All rights reserved.


nit: s/2016/2017/

gmlewis · 2017-02-02T19:11:25Z

+	}
+
+	req, err := s.client.NewRequest("GET", u, nil)
+


nix blank line between API call and err check

gmlewis · 2017-02-02T19:12:38Z

+	// TODO: remove custom Accept header when this API fully launches.
+	req.Header.Set("Accept", mediaTypeOrgMembershipPreview)
+
+	members := new([]*User)


var members []*User resp, err := s.client.Do(req, &members) if err != nil { return nil, resp, err } return members, resp, nil

I don't see these changes... do you need to push them?

gmlewis · 2017-02-02T19:13:01Z

@@ -0,0 +1,46 @@
+// Copyright 2016 The go-github AUTHORS. All rights reserved.


nit: s/2016/2017/

gmlewis · 2017-02-02T19:14:50Z

+	defer teardown()
+
+	mux.HandleFunc("/orgs/o/outside_collaborators", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")


please add test for preview header... see other examples in repo

gmlewis · 2017-02-02T19:16:23Z

+// Warning: The API may change without advance notice during the preview period.
+// Preview features are not supported for production use.
+//
+// GitHub API docs: https://developer.github.com/v3/orgs/outside_collaborators/


Change URL to:
https://developer.github.com/v3/orgs/outside_collaborators/#list-outside-collaborators

glapul · 2017-02-03T21:08:03Z

Done.

gmlewis · 2017-02-03T22:06:48Z

 	}

-	return *members, resp, err
+	return members, resp, err


return members, resp, nil

gmlewis

Thank you, @glapul!

gmlewis · 2017-02-03T22:23:21Z

Awaiting second LGTM.

dmitshur

LGTM. /cc @gmlewis

See a couple minor questions/comments inline.

dmitshur · 2017-02-13T23:02:51Z

+// user is an owner of the organization.
+//
+// Warning: The API may change without advance notice during the preview period.
+// Preview features are not supported for production use.


We don't usually include this warning in all other similar situations. Should we start doing it here?

It'll create more work and might get out of date. We have the // TODO: remove custom Accept header when this API fully launches. comments in code, and GitHub API itself is a good source for knowing which endpoints are stable or not.

I'm not against it, just a thought, because this is the first comment of its kind.

It's not the first one, I copied it from another file using the same Accept header. (and thought that's the convention). I'll find the file when I'll get back home.

Oh okay, keeping it is perfectly fine then. Thanks!

dmitshur · 2017-02-13T23:06:30Z

+
+// ListOutsideCollaborators lists outside collaborators of organization's repositories.
+// This will only work if the authenticated
+// user is an owner of the organization.


Nitpick, the sentence "This will only work if the authenticated user is an owner of the organization." can probably fit on a single line, instead of being split into two lines of lengths 40 and 37 characaters.

Also, question, what's the source of this statement? I don't see it mentioned in https://developer.github.com/v3/orgs/outside_collaborators/#list-outside-collaborators.

While trying to use the API by hand I was informed (with an error) that only the owner can do this.

gmlewis · 2017-02-22T05:48:59Z

It looks like this was ready to merge and I think I dropped the ball by not getting it in before the context changes.

I'm thinking that while I'm rebasing, that I could add a commit before squashing that adds context.

Does that sound reasonable to you, @shurcooL, or do you have an alternative proposal?

dmitshur · 2017-02-22T06:05:12Z

Yes, this PR is just 1 logical commit + 3 adjustments based on review comments. I think you should rebase them, add context support and squash everything into one commit before merging into master.

Add more commits by pushing to the outside_collaborators branch on glapul/go-github.

You can also do that.

gmlewis · 2017-02-22T06:11:23Z

SGTM. Adjusting and merging.

gmlewis · 2017-02-22T06:21:02Z

Integrated in 93f95bf

2fa_disabled filter is supported too. Closes google#538. Change-Id: I7513435eebd2a20d2392cdb375818b3c8e80accd

Added listing outside collaborators for an organization

3cd8f92

2fa_disabled filter is supported too.

glapul mentioned this pull request Feb 1, 2017

Support preview Organizations Membership API and new filter #524

Closed

gmlewis requested changes Feb 2, 2017

View reviewed changes

Addressing requests for changes.

ae25b35

Adressing the requests for changes, part 2

a304877

gmlewis reviewed Feb 3, 2017

View reviewed changes

Adressing the requests for changes, part 3

7a5c237

gmlewis approved these changes Feb 3, 2017

View reviewed changes

dmitshur self-assigned this Feb 13, 2017

dmitshur approved these changes Feb 13, 2017

View reviewed changes

dmitshur removed their assignment Feb 13, 2017

gmlewis closed this in 93f95bf Feb 22, 2017

dmitshur mentioned this pull request Feb 22, 2017

Update API inside documentation. #561

Closed

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Added listing outside collaborators for an organization

d610642

2fa_disabled filter is supported too. Closes google#538. Change-Id: I7513435eebd2a20d2392cdb375818b3c8e80accd

		@@ -0,0 +1,51 @@
		// Copyright 2016 The go-github AUTHORS. All rights reserved.

		@@ -0,0 +1,46 @@
		// Copyright 2016 The go-github AUTHORS. All rights reserved.

Conversation

glapul commented Feb 1, 2017

Uh oh!

gmlewis commented Feb 1, 2017

Uh oh!

dmitshur commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Feb 1, 2017

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glapul commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Feb 3, 2017

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Feb 22, 2017

Uh oh!

dmitshur commented Feb 22, 2017

Uh oh!

gmlewis commented Feb 22, 2017

Uh oh!

gmlewis commented Feb 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmitshur commented Feb 1, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading