Move the GitHub Teams API#1400
Conversation
googlebot
added
the
cla: yes
|
I think this PR is missing (some?) of the new Team API endpoints e. g. fetching a team by it's slug |
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thanks, @Atorr, but this PR is attempting to do way too much and can not be fully reviewed as written.
As described below, please address the comments reviewed so far, and revert all the unnecessary changes performed in reorganizing the file structure.
Also, please to not use "force push" for any future commits, as that will waste a great deal of time when reviewing. Thanks.
| // GitHub API docs: https://developer.github.com/v3/teams/#get-team | ||
| func (s *TeamsService) GetTeam(ctx context.Context, team int64) (*Team, *Response, error) { | ||
| u := fmt.Sprintf("teams/%v", team) | ||
| // Github API docs: https://developer.github.com/v3/teams/#get-team-by-name |
| } | ||
|
|
||
| // DeleteTeam deletes a team. | ||
| // EditTeamByName edits a team, given an organiation name, by name. |
There was a problem hiding this comment.
For consistency, please rename this function to EditTeamBySlug and change comment to say "by slug."
| // DeleteTeamByName deletes a team reference by Name. | ||
| // | ||
| // GitHub API docs: https://developer.github.com/v3/teams/#delete-team | ||
| func (s *TeamsService) DeleteTeamByName(ctx context.Context, org, slug string) (*Response, error) { |
There was a problem hiding this comment.
For consistency, please rename this function to DeleteTeamBySlug and change comment to day "reference by slug."
| } | ||
|
|
||
| // ListTeamRepos lists the repositories that the specified team has access to. | ||
| // ListChildTeamsByParentName lists child teams for a parent team given parent name. |
There was a problem hiding this comment.
For consistency, please rename this function to ListChildTeamsByParentSlug, and change the comment to say "given parent slug".
| return repos, resp, nil | ||
| } | ||
|
|
||
| // ListTeamReposByName lists the repositories given a team name that the specified team has access to. |
There was a problem hiding this comment.
ListTeamReposBySlug... given a team slug.
| return s.client.Do(ctx, req, nil) | ||
| } | ||
|
|
||
| // RemoveTeamRepoByName removes a repository from being managed by the specified |
There was a problem hiding this comment.
RemoveTeamRepoBySlug...given the team slug.
| return projects, resp, nil | ||
| } | ||
|
|
||
| // ListTeamProjectsByName lists the organization projects for a team given the team name. |
There was a problem hiding this comment.
ListTeamProjectsBySlug... given the team slug.
| // as an organization member, the authenticated user must have "read" access to both the team | ||
| // and project, or "admin" access to the team or project. | ||
| // Note: This endpoint removes the project from the team, but does not delete it. | ||
| // AddTeamProjectByName adds an organization project to a team given the team name. |
There was a problem hiding this comment.
AddTeamProjectBySlug...given the team slug.
|
|
||
| // CreateOrUpdateIDPGroupConnections creates, updates, or removes a connection between a team | ||
| // and an IDP group. | ||
| // RemoveTeamProjectByName removes an organization project from a team given team name. |
There was a problem hiding this comment.
RemoveTeamProjectBySlug...given team slug.
| return s.client.Do(ctx, req, nil) | ||
| } | ||
|
|
||
| // IDPGroupList represents a list of external identity provider (IDP) groups. |
There was a problem hiding this comment.
Why are lines 497 to the end of the file being deleted in this PR?
Oh, it looks like you are also trying to reorganize the files in this PR as well?
Sorry, but please handle file reorganization in a separate PR.
Attempting to address the changes listed in the announcement: https://developer.github.com/changes/2020-01-21-moving-the-team-api-endpoints/
while at the same time reorganizing files and moving code around makes this PR much much larger than it needs to be and is not reviewable as such.
So please keep this PR focused just on the checklist I made in #1387 and revert the rest of the changes and make a separate PR for those. Thanks.
There was a problem hiding this comment.
Sure thing. Must've missed the list. Working on reverting the changes now. Thanks!
…m members and team synchronization endpoints, and make fixes per PR review
|
New changes in. Had missed the link to the blog post and went a bit overboard. Will work to pay more attention to what needs to be changed. Thank you all for your patience on this! |
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Hey @Atorr ... this looks fantastic now!
Thank you so much for all this work you've done! It is tremendously appreciated by myself and by the hundreds (or maybe even thousands) of happy, silent users of this client library! 😄
LGTM.
Awaiting second LGTM before merging.
|
Is there any chance to get that merged/released soonish? |
|
Hi @weeco - this repo is maintained by volunteers and it is difficult to tell when people will have time to review PRs, so I don't have an ETA for you. I suggest that if you need something right away, you fork the repo and pull in the PR(s) that you need and work off of that. Thank you for your patience. |
|
Thank you, @wesleimp! |
|
👋 @gmlewis I have a request 🙏 can you post a comment in this PR when there's a release that includes this change? I'd appreciate it tons ✨ |
|
@lynncyrin - https://github.com/google/go-github/releases/tag/v29.0.3 incorporates this change. |
6 participants
Fixes: #1387.