Conversation
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
tolusha
changed the title
tolusha
requested review from
SDawley,
dkwon17,
ibuziuk and
mkuznyetsov
as code owners
ibuziuk
left a comment
ibuziuk
left a comment
There was a problem hiding this comment.
Great job, but let's postpone the merge and have it in 7.116.0 for 3.28
| os.Exit(1) | ||
| } | ||
|
|
||
| if hasAPIGroup(apiGroups, "route.openshift.io") { |
There was a problem hiding this comment.
We’re detecting OpenShift via the presence of the route.openshift.io API group.
Some OpenShift clusters (like microshift) may not expose routes, as it's not a core APIGroup. Would checking config.openshift.io or oauth.openshift.io be more robust?
| "--config=/etc/oauth-proxy/oauth-proxy.cfg", | ||
| "--ping-path=/ping", | ||
| "--exclude-logging-path=/ping", | ||
| } |
There was a problem hiding this comment.
Nit, --config=/etc/oauth-proxy/oauth-proxy.cfg is duplicated in both branches. Do you think it's worth initializing a base args slice with the common config and appending conditionally?
| func hasAPIGroup(source []*metav1.APIGroup, apiName string) bool { | ||
| for i := 0; i < len(source); i++ { | ||
| if source[i].Name == apiName { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
You can use slices.ContainsFunc from the slices package (available since Go 1.21)
| func hasAPIGroup(source []*metav1.APIGroup, apiName string) bool { | |
| for i := 0; i < len(source); i++ { | |
| if source[i].Name == apiName { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| import "slices" | |
| func hasAPIGroup(source []*metav1.APIGroup, apiName string) bool { | |
| return slices.ContainsFunc(source, func(g *metav1.APIGroup) bool { | |
| return g.Name == apiName | |
| }) | |
| } |
| func GetGatewayKubernetesAuthenticationSidecarImage(checluster interface{}) string { | ||
| if !initialized { | ||
| logrus.Fatalf("Operator defaults are not initialized.") | ||
| } |
There was a problem hiding this comment.
Sorry, I might not have the full context, but this package’s use of logrus.Fatalf in getters looks very unusual.
If the intent is for the operator to fail fast on misconfiguration, would it make sense to centralize the fatal behavior in Initialize() instead of spreading it across getters?
| Unknown Type = iota | ||
| Kubernetes | ||
| OpenShiftV4 | ||
| OpenShiftV5 |
There was a problem hiding this comment.
I might be missing some context, but it looks like OpenShiftV5 is defined in the enum but never assigned at runtime.
I understand it's for forward compatibility. I suggest adding a small comment to make that explicit.
3 participants
What does this PR do?
Support OpenShift external IDP
Screenshot/screencast of this PR
What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-9763
How to test this PR?
eclipse-che/che-docs#3024
Common Test Scenarios
PR Checklist
As the author of this Pull Request I made sure that:
Reviewers
Reviewers, please comment how you tested the PR when approving it.