+
Skip to content

Update IdP team-sync list and create/update endpoints with ByID and BySlug #1484

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

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

anGie44
Copy link
Contributor

@anGie44 anGie44 commented Apr 8, 2020

Addresses Issue #1483
Changes include:

  • Update method signatures of #ListIDPGroupsForTeam and #CreateOrUpdateIDPGroupConnections to include the organization name identified as org and rename the teamID attribute to slug as it can be misleading/the api requires the slug for these particular calls
  • Update respective tests with corrected arguments

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@codecov
Copy link

codecov bot commented Apr 8, 2020

Codecov Report

Merging #1484 into master will decrease coverage by 0.04%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1484      +/-   ##
==========================================
- Coverage   68.16%   68.11%   -0.05%     
==========================================
  Files          94       94              
  Lines        8436     8462      +26     
==========================================
+ Hits         5750     5764      +14     
- Misses       1823     1831       +8     
- Partials      863      867       +4     
Impacted Files Coverage Δ
github/teams.go 66.89% <60.00%> (-0.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a7cf4a...ebe1c40. Read the comment docs.

@anGie44
Copy link
Contributor Author

anGie44 commented Apr 8, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Apr 8, 2020
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @anGie44, but I would like to handle this situation similarly to how we did #1400
where we now provide two methods for each of the two possible endpoints listed in the GitHub docs.

Specifically, we would have ListIDPGroupsForTeamByID and ListIDPGroupsForTeamBySlug.
Similarly, we would have CreateOrUpdateIDPGroupConnectionsByID and CreateOrUpdateIDPGroupConnectionsBySlug.

You did the BySlug versions, but please rename these and add in the ByID versions as well.

Thank you!

@anGie44
Copy link
Contributor Author

anGie44 commented Apr 8, 2020

Hi @gmlewis, thank you for the feedback! I'm totally onboard with supporting both endpoint versions. I'll submit a change shortly :)

@anGie44 anGie44 requested a review from gmlewis April 9, 2020 00:16
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @anGie44 !
LGTM.

Awaiting second LGTM before merging.

Copy link
Collaborator

@wesleimp wesleimp left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 9, 2020

Thank you, @wesleimp!
Merging.

Note to self: this is a breaking API change which means that the next release must be v31.0.0.

Note to others: we only make new releases now on-demand which means that we will continue to address issues and update the repo without releasing until someone says "Hey, we want to use the feature in that latest PR for this huge project and we would like to please have a new release" or something to that effect. 😄

@gmlewis gmlewis changed the title [ISSUE-1483] prepend IdP team-sync list and create/update calls with org name Update IdP team-sync list and create/update endpoints with ByID and BySlug Apr 9, 2020
@gmlewis gmlewis merged commit 1513a77 into google:master Apr 9, 2020
@anGie44
Copy link
Contributor Author

anGie44 commented Apr 9, 2020

Hi @gmlewis thanks again for reviewing this PR! wanted to follow-up about having this change in a future release. in the terraform-provider-github project, I have a recent PR to support team synchronization and this change would help make this possible :)

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 9, 2020

OK, @anGie44, I have another breaking API change coming up in #1477 and it would be great to get that in at the same time so we don't have to create v31.0.0 immediately followed with v32.0.0 a few days later...

Let me pull in the big guns to make this decision... hold on a sec... 😄

@willnorris - what do you think about getting #1477 merged before we bump the version to v31.0.0? That one involves breaking API changes too because it found a bunch of receivers that were not pointers, but instead were being passed by value.

@anGie44
Copy link
Contributor Author

anGie44 commented Apr 9, 2020

thank you, @gmlewis! I really appreciate the quick response :) On my end, it's not drastically time sensitive so I'm happy to wait for the other PRs to get merged for this next release.

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载