Decouple logic to detect OpenShift from OpenShift Routes. #1338 #1421
Conversation
…try#1338 Signed-off-by: Israel Blancas <[email protected]>
|
/cc @frzifus |
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
| @@ -0,0 +1,16 @@ | |||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | |||
| change_type: bug_fix | |||
There was a problem hiding this comment.
Is this a user-facing bug fix or fix of the e2e test?
I would prefer to skip changelog for fixing tests
There was a problem hiding this comment.
Is not a test fix.
I cataloged it as bug fix because installing the OpenShift Routes in a Kubernetes cluster, will make the operator think it is an OpenShift server.
pkg/openshift-routes/main.go
Outdated
| package openshift_routes | ||
|
|
||
| // Platform holds the auto-detected platform type. | ||
| type OpenShiftRoutesAvailability int |
There was a problem hiding this comment.
I don't get the purpose of this package. Is it just to define the constants?
There was a problem hiding this comment.
Alright this package seems to be inspired by the platform package. I would propose to merge the platform package with the autodetect. The consumer of platform needs to import autodetect as well, I don't see a reason why these packages need to be split.
There was a problem hiding this comment.
@frzifus What I wanted to say: as @pavolloffay, I followed the implementation platform. Following your directions during our conversation talking about #1338.
pkg/openshift-routes/main.go
Outdated
|
|
||
| const ( | ||
| // Is not clear if OpenShift Routes are available. | ||
| Unknown OpenShiftRoutesAvailability = iota |
There was a problem hiding this comment.
Why do we need both Unknown and NotAvailable states?
If possible I would define only Available and NotAvailable
There was a problem hiding this comment.
Followed the approach suggested by @frzifus. Copied from the platform approach
There was a problem hiding this comment.
From my point of view it make sense to provide an unknown state too. Even it its the same as NotAvailable it provides the information that until then, the detection didnt run.
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
Signed-off-by: Israel Blancas <[email protected]>
iblancasa
requested review from
pavolloffay and
frzifus
and removed request for
pavolloffay and
frzifus
iblancasa
requested review from
pavolloffay and
frzifus
and removed request for
pavolloffay and
frzifus
Signed-off-by: Israel Blancas [email protected]
Fixes #1418