Skip to content

Add support for SAML SSO authorization APIs - #2835

Merged
gmlewis merged 4 commits into
google:masterfrom
joaopenteado:credential-authorizations
Jul 24, 2023
Merged

Add support for SAML SSO authorization APIs#2835
gmlewis merged 4 commits into
google:masterfrom
joaopenteado:credential-authorizations

Conversation

@joaopenteado

Copy link
Copy Markdown
Contributor

This PR adds support for both ListCredentialAuthorizations and RemoveCredentialAuthorization methods available for the GitHub Enterprise Cloud REST API for SAML SSO authorizations.

Listing and deleting credential authorizations is available to organizations with GitHub Enterprise Cloud.

@joaopenteadojoaopenteado changed the title Added support for SAML SSO authorization APIsAdd support for SAML SSO authorization APIsJul 20, 2023

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @joaopenteado !
Just a few tweaks, please, then we will be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment threadgithub/orgs_credential_authorizations.go
Comment threadgithub/orgs_credential_authorizations_test.go
Comment threadgithub/orgs_credential_authorizations.go Outdated
Comment threadgithub/orgs_credential_authorizations.go Outdated
@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Jul 20, 2023
joaopenteadoand others added 2 commits July 21, 2023 10:24
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@codecov

codecovBot commented Jul 21, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2835 (529eb79) into master (b02bb75) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2835 +/- ##
=======================================
Coverage 98.06% 98.06% =======================================
Files 137 138 +1 Lines 12291 12314 +23 =======================================
+ Hits 12053 12076 +23 
Misses 162 162 Partials 76 76 
Impacted FilesCoverage Δ
github/orgs_credential_authorizations.go100.00% <100.00%> (ø)

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @joaopenteado !
LGTM.

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

@joaopenteado

Copy link
Copy Markdown
ContributorAuthor

Thank you for the fast review and comments as always, @gmlewis!
I just committed a improvement to the test for ListCredentialAuthorizations in order to match what was being done in other functions.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome, @joaopenteado ! Thank you very much!
I was wondering why the coverage dropped, but it looks like you found it and fixed it!
🎉

@o-sama

Copy link
Copy Markdown
Contributor

LGTM too :)

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 24, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @o-sama !
Merging.

@gmlewis
gmlewis merged commit a3dc9cb into google:masterJul 24, 2023
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@joaopenteado@o-sama@gmlewis