-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add support for more than 20 Okta groups #106
Conversation
ed14b19
to
f23e25c
Compare
Code looks good to my eyeballs, @klofton-bw promises me that the new addition the CRD is optional, and if not set, the default is 1000 groups. Nice work |
README.md
Outdated
@@ -442,6 +391,58 @@ Additional metadata based on Keycloak group are also added to the OpenShift grou | |||
* Parent/child relationship between groups and their subgroups | |||
* Group attributes | |||
|
|||
### Keycloak |
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.
Can this be moved above Okta to retain alphabetic order
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, removed commit where I moved it
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.
One final change
api/v1alpha1/groupsync_types.go
Outdated
@@ -360,6 +360,10 @@ type OktaProvider struct { | |||
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Profile Key",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"} | |||
// +kubebuilder:validation:Optional | |||
ProfileKey string `json:"profileKey"` | |||
// GroupLimit is the maximum number of groups that can be synced. Default is "1000" | |||
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Group Limit",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"} |
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.
Since this is an integer, can we use urn:alm:descriptor:com.tectonic.ui:number
since it is an integer?
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
There was an issue with the Okta syncer due to the way the API call was made and how the client returned the groups in pages. This fix makes the number of groups the syncer can pull customizable with a default of 1000. Fixes issue #105