-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Implement SCIM #2062
Implement SCIM #2062
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2062 +/- ##
==========================================
+ Coverage 97.75% 97.76% +0.01%
==========================================
Files 107 108 +1
Lines 9600 9659 +59
==========================================
+ Hits 9384 9443 +59
Misses 150 150
Partials 66 66
Continue to review full report at Codecov.
|
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.
Whups, I now see that this is a draft... sorry for the premature code review. Well, I hope this helps anyway.
Thank you, it really helps. I will go through and fix it. |
I have updated the changes you mentioned, let me know if more changes are required. Thank you. |
fixed, let me know if need more changes. Thank you. |
github/scim.go
Outdated
// SCIMUserName represents SCIM user information. | ||
type SCIMUserName struct { | ||
GivenName string `json:"given_name"` // The first name of the user. (Required.) | ||
FamilyName string `json:"family_name"` // The last name of the user. (Required.) |
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.
Also, in some places, https://docs.github.com/en/rest/reference/scim#supported-scim-user-attributes says this should be familyName
and in other places it says it should be lastName
... this looks like a bug in the GitHub API docs, actually.
But maybe we should assume that this should be familyName
here since that is listed more times than lastName
?
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.
yeah make sense, I will update that.
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.
Are you able to test this out against a live instance of GitHub (either public or enterprise)?
If so, it would be great to get confirmation that this is working as intended.
github/scim.go
Outdated
// SCIMUserName represents SCIM user information. | ||
type SCIMUserName struct { | ||
GivenName string `json:"givenName"` // The first name of the user. (Required.) | ||
LastName string `json:"lastName"` // The last name of the user. (Required.) |
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.
In some places, https://docs.github.com/en/rest/reference/scim#supported-scim-user-attributes says this should be familyName
and in other places it says it should be lastName
... this looks like a bug in the GitHub API docs, actually. I'm betting that GitHub tech support should be contacted about this documentation problem.
But maybe we should assume that this should be familyName
here since that is listed more times than lastName
?
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.
yes, I misread that. I will change to use the familyName
. I am not sure how the support will work in the case of GitHub, does a personal account also comes with support? Or is this something you can do?
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.
I am not sure how the support will work in the case of GitHub, does a personal account also comes with support? Or is this something you can do?
I actually don't know. No, not that I'm aware of.
I don't have an enterprise version. Could I work with my personal account for this, will it work? I thought this will be for the enterprise ones since it is related to user management. |
Sorry, I don't know... you may be right. If so, that's fine. I'm sure people will file issues if there are problems. 😂 |
sounds good. Can get on as people file the issues. I will push the changes for the above we discussed, anything else needs to change? |
I can't think of anything else at the moment, thank you. |
Fixed. Thank you. |
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, @krishnaindani !
LGTM.
Awaiting second LGTM before merging.
Thank you. |
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, @Parker77 ! |
API Docs: https://docs.github.com/en/rest/reference/scim#list-scim-provisioned-identities--parameters
Fixes: #2030.