Skip to content
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

feat!: Add login query param support to ListCredentialAuthorizations #3270

Conversation

zeusdeux
Copy link
Contributor

@zeusdeux zeusdeux commented Sep 17, 2024

BREAKING CHANGE: ListCredentialAuthorizations now takes opts *CredentialAuthorizationsListOptions instead of ListOptions.

As per the docs for the List SAML SSO authorizations for an organization endpoint — it supports a query param named login to filter the authorizations by a github user.

This PR adds support for passing this login query param via a new struct (CredentialAuthorizationsListOptions) which embeds ListOptions as ListOptions itself only supports Page and PerPage query params.

// such as Page and PerPage
// GitHub API docs: https://docs.github.com/enterprise-cloud@latest/rest/orgs/orgs#list-saml-sso-authorizations-for-an-organization
type CredentialAuthorizationsListOptions struct {
ListOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose to embed ListOptions instead of extending it as it is reused for other List* functions which don't necessarily support the login query parameter.

@gmlewis gmlewis changed the title feat: Add login query param support to ListCredentialAuthorizations feat!: Add login query param support to ListCredentialAuthorizations Sep 17, 2024
@gmlewis gmlewis added Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging. labels Sep 17, 2024
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (2b8c7fa) to head (c849e6f).
Report is 113 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3270      +/-   ##
==========================================
- Coverage   97.72%   92.94%   -4.78%     
==========================================
  Files         153      171      +18     
  Lines       13390    11669    -1721     
==========================================
- Hits        13085    10846    -2239     
- Misses        215      729     +514     
- Partials       90       94       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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, @zeusdeux!
One tiny nit, please, for the generated godocs, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

github/orgs_credential_authorizations.go Outdated Show resolved Hide resolved
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@zeusdeux
Copy link
Contributor Author

Thanks for the quick review @gmlewis! Accepted your suggestion!

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, @zeusdeux !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@zeusdeux
Copy link
Contributor Author

@gmlewis should I tag someone as a second reviewer?

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2024

@gmlewis should I tag someone as a second reviewer?

Since this repo is maintained strictly by volunteers, I try to not tag individuals to respect their time unless there is an urgent need.
Having said that, please feel free to perform code reviews on any PRs that have been labeled "NeedsReview".

@zeusdeux
Copy link
Contributor Author

Sounds good @gmlewis! Thank you for the information.
Btw, re: failing build — any notes on the codecov/project failure? Not sure if I should take that into account given the test change in this PR covers the new code path. 🙏🏽

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 19, 2024

Btw, re: failing build — any notes on the codecov/project failure? Not sure if I should take that into account given the test change in this PR covers the new code path. 🙏🏽

Unfortunately, you can ignore the Codecov report, as we are waiting for #3221 to be addressed by @google-admin and update this repo's Codecov authentication token so that our reports will be useful.

@zeusdeux
Copy link
Contributor Author

Makes sense. Does that mean that this PR only needs one more ✅ and it'll be good to land?
And if so, what's the release process? Thanks for all the help so far @gmlewis! 🙏🏽

Copy link

@alexef alexef 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 gmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 19, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Sep 19, 2024

Does that mean that this PR only needs one more ✅ and it'll be good to land?

Yes, and it is now ready to merge. Thank you, @alexef !

And if so, what's the release process?

We cut a new release about once per month, so the next release should be in three to four weeks.

Merging.

@gmlewis gmlewis merged commit ef170a3 into google:master Sep 19, 2024
6 of 7 checks passed
@zeusdeux
Copy link
Contributor Author

Thank you @gmlewis and @alexef! 🙏🏽

@zeusdeux zeusdeux deleted the add-login-query-param-support-to-ListCredentialAuthorizations branch September 19, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants