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 ListProvisionedScimGroupsForEnterprise inside SCIM service with test #3467

Merged
merged 5 commits into from
Feb 12, 2025

Conversation

cyberious
Copy link
Contributor

This pull request introduces several enhancements to the SCIM (System for Cross-domain Identity Management) functionality in the github/scim.go file. The key changes include the addition of new types to represent SCIM group attributes and provisioned groups, as well as a new method to list provisioned SCIM groups for an enterprise. Additionally, a corresponding test function has been added to ensure the correctness of the new method.

Enhancements to SCIM functionality:

  • Added SCIMGroupAttributes type to represent supported SCIM group attributes, including DisplayName, Members, Schemas, ExternalID, ID, and Meta. ([github/scim.goR20-R38](https://github.com/google/go-github/pull/3467/files#diff-c2dda2100fd0c700da11a5d96582391b4923658db7e717627d248ae1c029c5fbR20-R38))
  • Introduced SCIMProvisionedGroups type to represent the result of calling ListSCIMProvisionedIdentities, including Schemas, TotalResults, ItemsPerPage, StartIndex, and Resources. ([github/scim.goR78-R86](https://github.com/google/go-github/pull/3467/files#diff-c2dda2100fd0c700da11a5d96582391b4923658db7e717627d248ae1c029c5fbR78-R86))
  • Added ListProvisionedScimGroupsForEnterprise method to list provisioned SCIM groups for an enterprise, including the necessary API documentation reference. ([github/scim.goR248-R269](https://github.com/google/go-github/pull/3467/files#diff-c2dda2100fd0c700da11a5d96582391b4923658db7e717627d248ae1c029c5fbR248-R269))

Testing:

  • Added TestSCIMService_ListSCIMProvisionedGroups function to test the new ListProvisionedScimGroupsForEnterprise method, ensuring it handles responses correctly and validates various scenarios. ([github/scim_test.goR124-R217](https://github.com/google/go-github/pull/3467/files#diff-2723dd506a61e0f7abe849217ded7c4aa5dfef10455f14ed10a423c65f4dac49R124-R217))

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.04%. Comparing base (ce42642) to head (3781247).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3467   +/-   ##
=======================================
  Coverage   91.03%   91.04%           
=======================================
  Files         179      179           
  Lines       15538    15551   +13     
=======================================
+ Hits        14145    14158   +13     
  Misses       1221     1221           
  Partials      172      172           

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

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 8, 2025

Please run step 4 of CONTRIBUTING.md and push the results (./script/generate.sh, ./script/lint.sh, and ./script/test.sh).

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Feb 8, 2025
@cyberious
Copy link
Contributor Author

cyberious commented Feb 8, 2025

@gmlewis updated it. I apologize for not seeing that earlier.

Squashed into the same commit

@cyberious cyberious changed the title Add ListProvisionedScimGroupsForEnterprise inside SCIM service with test feat: Add ListProvisionedScimGroupsForEnterprise inside SCIM service with test Feb 8, 2025
@gmlewis
Copy link
Collaborator

gmlewis commented Feb 8, 2025

Just FYI - As described in CONTRIBUTING.md, there is no need to force-push in this repo, as we always squash-and-merge so that the commit history is clean. In fact, we discourage it because it makes it more challenging for reviewers to see what changed since their last review.
However, for this tiny PR, it really makes no difference, so no big deal... but just wanted to let you know in case you make larger PRs in the future, which are totally welcome. 😄 ❤️

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, @cyberious !
One minor tweak, please, then awaiting second LGTM+Approval from any other contributor to this repo before merging.

@google google deleted a comment from ohpintu90 Feb 9, 2025
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, @cyberious !
LGTM

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

@stevehipwell - might you have time for a code review? Thank you!

Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

Minor nit/question.

Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

LGTM

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, @cyberious and @stevehipwell !
LGTM
Merging.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Feb 11, 2025
@gmlewis gmlewis merged commit 77684a4 into google:master Feb 12, 2025
7 checks passed
Sign up for free to 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