-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
…lls with organization name
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this 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!
Hi @gmlewis, thank you for the feedback! I'm totally onboard with supporting both endpoint versions. I'll submit a change shortly :) |
… team id and slug
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👌
Thank you, @wesleimp! Note to self: this is a breaking API change which means that the next release must be 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. 😄 |
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 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 |
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. |
Addresses Issue #1483
Changes include:
#ListIDPGroupsForTeam
and#CreateOrUpdateIDPGroupConnections
to include the organization name identified asorg
and rename theteamID
attribute toslug
as it can be misleading/the api requires the slug for these particular calls