Skip to content

Add support for SAML SSO authorization APIs#2835

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

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

Conversation

@joaopenteado
Copy link
Copy Markdown
Contributor

@joaopenteado joaopenteado commented Jul 20, 2023

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.

@joaopenteado joaopenteado changed the title Added support for SAML SSO authorization APIs Add support for SAML SSO authorization APIs Jul 20, 2023
gmlewis
gmlewis requested changes Jul 20, 2023
View reviewed changes
Copy link
Copy Markdown
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, @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.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jul 20, 2023
joaopenteado and others added 2 commits July 21, 2023 10:24
@joaopenteado @gmlewis
Applied suggestions from code review
cfba85a 
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@joaopenteado
Added omitempty to CredentialAuthorization struct members
756d7b4
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 21, 2023 •
edited
Loading

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 Files Coverage Δ
github/orgs_credential_authorizations.go 100.00% <100.00%> (ø)

gmlewis
gmlewis approved these changes Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
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, @joaopenteado !
LGTM.

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

@joaopenteado
Copy link
Copy Markdown
Contributor Author

joaopenteado commented Jul 21, 2023

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.

gmlewis
gmlewis approved these changes Jul 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

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

o-sama commented Jul 23, 2023

LGTM too :)

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jul 24, 2023
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Jul 24, 2023

Thank you, @o-sama !
Merging.

@gmlewis gmlewis merged commit a3dc9cb into google:master Jul 24, 2023
@gmlewis gmlewis mentioned this pull request Aug 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis approved these changes

+1 more reviewer

@o-sama o-sama o-sama approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joaopenteado @o-sama @gmlewis