Conversation
google-cla
bot
added
the
cla: yes
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.
|
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
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.
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.
yeah make sense, I will update that.
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
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.
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.
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.
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. |
gmlewis
left a comment
gmlewis
left a comment
There was a problem hiding this comment.
Thank you, @krishnaindani !
LGTM.
Awaiting second LGTM before merging.
Thank you. |
|
Thank you, @Parker77 ! |
3 participants
API Docs: https://docs.github.com/en/rest/reference/scim#list-scim-provisioned-identities--parameters
Fixes: #2030.