Add parent team id param #724

elliott-beach · 2017-09-23T03:11:05Z

This replaces Team with NewTeam in function that accept a team for creating or editing a team.
The NewTeam struct is similar to the existing NewPullRequest for creating pull requests.

We need a new struct because the set of parameters for creating or editing a team contains some parameters that are not in the parameter for teams, and most properties of teams cannot be used for editing.

The specific change is that Teams are returned with a parent field, but are created/edited with a parent_id field.

dmitshur · 2017-09-23T15:57:15Z

github/orgs_teams.go

+	Name         *string   `json:"name,omitempty"`
+	Description  *string   `json:"description,omitempty"`
+	Maintainers  []*string `json:"maintainers,omitempty"`
+	RepoNames    []*string `json:"repo_names,omitempty"`


These two fields can be simplified to []string.

You can look at other structures for example. We use pointers to basic types so it’s possible to tell if the field was missing or zero. Slices are already pointer types and can be nil if missing.

dmitshur · 2017-09-23T16:00:10Z

github/orgs_teams.go

+type NewTeam struct {
+	ID           *int      `json:"id,omitempty"`
+	Name         *string   `json:"name,omitempty"`
+	Description  *string   `json:"description,omitempty"`


Are any of these fields required rather than optional?

If so, since this struct is used for providing input only, we can use values for mandatory fields.

Yes, the ID field is required. What you are suggesting makes sense.

Great.

If you want to see some examples, see RequiredStatusChecks and grep for "Required" and "Optional" in comments.

dmitshur · 2017-09-23T16:00:44Z

github/orgs_teams_test.go

@@ -73,6 +73,7 @@ func TestOrganizationService_GetTeam_nestedTeams(t *testing.T) {
 		testHeader(t, r, "Accept", mediaTypeNestedTeamsPreview)
 		fmt.Fprint(w, `{"id":1, "name":"n", "description": "d", "url":"u", "slug": "s", "permission":"p",
 		"parent": {"id":2, "name":"n", "description": "d", "parent": null}}`)
+


Was this line added by accident?

dmitshur · 2017-09-23T16:02:31Z

Thanks for providing the rationale for NewTeam struct, it sounds justified. 👍

alindeman · 2017-09-24T00:40:12Z

Related: #714

Looking forward to having this. I will plan to use it to implement parent_team_id in the Terraform GitHub provider unless someone else beats me to it :)

elliott-beach · 2017-09-25T04:25:29Z

I noticed and fixed a mistake I made - Name is the required parameter, while ID should not even be a field of NewTeam.

elliott-beach · 2017-09-25T04:32:26Z

github/orgs_teams.go

@@ -139,7 +164,7 @@ func (s *OrganizationsService) CreateTeam(ctx context.Context, org string, team
 // EditTeam edits a team.
 //
 // GitHub API docs: https://developer.github.com/v3/orgs/teams/#edit-team
-func (s *OrganizationsService) EditTeam(ctx context.Context, id int, team *Team) (*Team, *Response, error) {
+func (s *OrganizationsService) EditTeam(ctx context.Context, id int, team *NewTeam) (*Team, *Response, error) {


It's odd that the EditTeam endpoint requires both a name and an id for the team, but that is the documented behavior.

alindeman · 2017-09-25T17:57:25Z

github/orgs_teams.go

-	// TODO: decide whether to delete
-	// LDAPDN is only available in GitHub Enterprise and when the team
-	// membership is synchronized with LDAP.
-	LDAPDN *string `json:"ldap_dn,omitempty"`


@e-beach I don't think LDAPDN should be deleted. On GitHub Enterprise, it's allowed to add or update a team with this parameter. We use it in the Terraform GitHub provider too.

Thanks for that feedback! I'll add it back in today, whoops.

Thanks @e-beach!

dmitshur

These are the only minor issues I see.

dmitshur · 2017-09-26T18:29:10Z

github/orgs_teams.go

+type NewTeam struct {
+
+	// The name of the team (Required.)
+	Name         string   `json:"name"`


Since there's no blank line after Name field, and Description doesn't have a comment of its own, this reads as if the comment above is for the whole block. You can make it inline to make it more readable. Also, maybe drop the "The" to shorten it.

Name string `json:"name"` // Name of the team. (Required.) Description *string `json:"description,omitempty"` ...

dmitshur · 2017-09-26T18:29:35Z

github/orgs_teams.go

@@ -114,10 +114,39 @@ func (s *OrganizationsService) GetTeam(ctx context.Context, team int) (*Team, *R
 	return t, resp, nil
 }

+// NewTeam represents a team to be created or modified.
+type NewTeam struct {
+


Is this blank line helpful? If not, let's remove it.

googlebot · 2017-09-26T21:07:52Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

alindeman · 2017-09-27T01:26:33Z

LGTM, but 511f540 being in the commit list is a bit odd. Maybe rebased from the wrong spot?

elliott-beach · 2017-09-27T01:28:20Z

Yes. Sorry - I was occupied and just got to trying to fix this. I rebased from master, but I think that is actually unnecessary. Not sure why there are conflicting files.

googlebot · 2017-09-27T01:39:12Z

CLAs look good, thanks!

elliott-beach · 2017-09-27T02:06:12Z

This should be good now.

I rebased off of master, realized that was unnecessary and caused conflicts, so rebased off of NestedTeamsPreview and pushed again.

@shurcooL your style suggestions should be addressed in 94ed0c7.

gmlewis

LGTM.
Thank you, @e-beach and @shurcooL!

dmitshur

I rebased off of master, realized that was unnecessary and caused conflicts, so rebased off of NestedTeamsPreview and pushed again.

That's the way to go. 👍

Just some really minor comments left.

dmitshur · 2017-09-27T18:42:40Z

github/orgs_teams.go

+	// using the new GitHub permission model. It no longer identifies the
+	// permission a team has on its repos, but only specifies the default
+	// permission a repo is initially added with. Avoid confusion by
+	// specifying a permission value when calling AddTeamRepo.


@e-beach Are you familiar with the convention Go uses for deprecated symbols?

I think it would be nice to use here.

That is copied from Team so I'll fix it there as well:

go-github/github/orgs_teams.go

Lines 23 to 28 in 511f540

// Permission is deprecated when creating or editing a team in an org

// using the new GitHub permission model. It no longer identifies the

// permission a team has on its repos, but only specifies the default

// permission a repo is initially added with. Avoid confusion by

// specifying a permission value when calling AddTeamRepo.

Permission *string `json:"permission,omitempty"`

That instance of Permission no longer needs the deprecation notice, however, as the deprecation is only for functions where NewTeam is now used.

dmitshur · 2017-09-27T18:43:47Z

github/orgs_teams.go

@@ -114,10 +114,37 @@ func (s *OrganizationsService) GetTeam(ctx context.Context, team int) (*Team, *R
 	return t, resp, nil
 }

+// NewTeam represents a team to be created or modified.
+type NewTeam struct {
+	Name         string   `json:"name"` // Name of the team (Required.)


Nitpicky, but I'd add a period at the end of the sentence. "Required" is another sentence inside parenthesis.

// Name of the team. (Required.)

This how it's written elsewhere. See:

go-github/github/repos.go

Line 552 in 511f540

// Require branches to be up to date before merging. (Required.)

elliott-beach · 2017-09-30T00:59:44Z

@shurcooL I believe I addressed your review.

dmitshur · 2017-10-03T02:53:50Z

Thanks! Those adjustments look great. My LGTM from before still applies. Merging this to NestedTeamsPreview branch.

This replaces Team with NewTeam in function that accept a team for creating or editing a team. The NewTeam struct is similar to the existing NewPullRequest for creating pull requests. We need a new struct because the set of parameters for creating or editing a team contains some parameters that are not in the parameter for teams, and most properties of teams cannot be used for editing. The specific change is that Teams are returned with a parent field, but are created/edited with a parent_id field. Helps #714.

* Add support for fetching parent team (for nested teams). (#723) Create mediaTypeNestedTeamsPreview const for https://developer.github.com/changes/2017-08-30-preview-nested-teams/. Add parent field to the Team struct. Set preview API media type in endpoints that return a team, so that the new parent field gets populated. Helps #714. * Add NewTeam struct. (#724) This replaces Team with NewTeam in function that accept a team for creating or editing a team. The NewTeam struct is similar to the existing NewPullRequest for creating pull requests. We need a new struct because the set of parameters for creating or editing a team contains some parameters that are not in the parameter for teams, and most properties of teams cannot be used for editing. The specific change is that Teams are returned with a parent field, but are created/edited with a parent_id field. Helps #714. * Add NestedTeamsPreview header to remaining endpoints Fixes #714. * Change ParentTeamID field to int (from string). (#748) Using the code as written in the `NestedTeamsPreview` branch produces an error when talking to the GitHub API: panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request. For 'properties/parent_team_id', "2514824" is not a number or null. [] The documentation at https://developer.github.com/v3/orgs/teams/#create-team states that this parameter is an id type which appears to mean an integer, not a string. Also, id is an integer in the sample responses for Get team endpoint, etc. * Bump libraryVersion * NestedTeamsPreview: Add the ListChildTeams endpoint (#758)

* Add support for fetching parent team (for nested teams). (google#723) Create mediaTypeNestedTeamsPreview const for https://developer.github.com/changes/2017-08-30-preview-nested-teams/. Add parent field to the Team struct. Set preview API media type in endpoints that return a team, so that the new parent field gets populated. Helps google#714. * Add NewTeam struct. (google#724) This replaces Team with NewTeam in function that accept a team for creating or editing a team. The NewTeam struct is similar to the existing NewPullRequest for creating pull requests. We need a new struct because the set of parameters for creating or editing a team contains some parameters that are not in the parameter for teams, and most properties of teams cannot be used for editing. The specific change is that Teams are returned with a parent field, but are created/edited with a parent_id field. Helps google#714. * Add NestedTeamsPreview header to remaining endpoints Fixes google#714. * Change ParentTeamID field to int (from string). (google#748) Using the code as written in the `NestedTeamsPreview` branch produces an error when talking to the GitHub API: panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request. For 'properties/parent_team_id', "2514824" is not a number or null. [] The documentation at https://developer.github.com/v3/orgs/teams/#create-team states that this parameter is an id type which appears to mean an integer, not a string. Also, id is an integer in the sample responses for Get team endpoint, etc. * Bump libraryVersion * NestedTeamsPreview: Add the ListChildTeams endpoint (google#758)

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Sep 23, 2017

dmitshur reviewed Sep 23, 2017

View reviewed changes

elliott-beach commented Sep 25, 2017

View reviewed changes

alindeman reviewed Sep 25, 2017

View reviewed changes

dmitshur approved these changes Sep 26, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Sep 26, 2017

elliott-beach force-pushed the add-parent-team-id-param branch from 3e35f60 to d526d52 Compare September 26, 2017 21:36

elliott-beach added 10 commits September 26, 2017 20:37

Add the NewTeam struct.

0346f43

Use NewTeam param in CreateTeam, EditTeam

9c6a714

Remove LDAPDN from NewTeam

3679a4c

remove whitespace

44ea27e

go generate

117c8b3

simplify string to string pointer

9763dd9

struct NewTeam: remove id, make Name required

eaa244b

add testHeader to EditTeam

d8dcf77

Add LDAPN to NewTeam

eb38245

fix style

94ed0c7

elliott-beach force-pushed the add-parent-team-id-param branch from d526d52 to 94ed0c7 Compare September 27, 2017 01:39

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Sep 27, 2017

gmlewis approved these changes Sep 27, 2017

View reviewed changes

dmitshur reviewed Sep 27, 2017

View reviewed changes

Add deprecation notice, style

239f3f6

add string convience method to NewTeam

235698d

dmitshur merged commit f748e16 into google:NestedTeamsPreview Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parent team id param #724

Add parent team id param #724

elliott-beach commented Sep 23, 2017

dmitshur Sep 23, 2017

dmitshur Sep 23, 2017

elliott-beach Sep 23, 2017

dmitshur Sep 23, 2017

dmitshur Sep 23, 2017

dmitshur commented Sep 23, 2017

alindeman commented Sep 24, 2017 •

edited

Loading

elliott-beach commented Sep 25, 2017

elliott-beach Sep 25, 2017

alindeman Sep 25, 2017

elliott-beach Sep 25, 2017

alindeman Sep 25, 2017

dmitshur left a comment

dmitshur Sep 26, 2017

dmitshur Sep 26, 2017

googlebot commented Sep 26, 2017

alindeman commented Sep 27, 2017

elliott-beach commented Sep 27, 2017

googlebot commented Sep 27, 2017

elliott-beach commented Sep 27, 2017

gmlewis left a comment

dmitshur left a comment

dmitshur Sep 27, 2017

elliott-beach Sep 27, 2017 •

edited

Loading

dmitshur Sep 27, 2017

dmitshur Sep 27, 2017

elliott-beach commented Sep 30, 2017

dmitshur commented Oct 3, 2017

	// Permission is deprecated when creating or editing a team in an org
	// using the new GitHub permission model. It no longer identifies the
	// permission a team has on its repos, but only specifies the default
	// permission a repo is initially added with. Avoid confusion by
	// specifying a permission value when calling AddTeamRepo.
	Permission *string `json:"permission,omitempty"`

Add parent team id param #724

Add parent team id param #724

Conversation

elliott-beach commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Sep 23, 2017

alindeman commented Sep 24, 2017 • edited Loading

elliott-beach commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Sep 26, 2017

alindeman commented Sep 27, 2017

elliott-beach commented Sep 27, 2017

googlebot commented Sep 27, 2017

elliott-beach commented Sep 27, 2017

gmlewis left a comment

Choose a reason for hiding this comment

dmitshur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliott-beach Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliott-beach commented Sep 30, 2017

dmitshur commented Oct 3, 2017

alindeman commented Sep 24, 2017 •

edited

Loading

elliott-beach Sep 27, 2017 •

edited

Loading