cmd/openshift-install/create: Use library-go's IngressURI helper #4245
Conversation
wking
force-pushed
the
library-go-route-ingress-uri
branch
2 times, most recently
from
f2f4bf1
to
6ea5e9c
Compare
|
/retest Prow was temporarily out of GitHub tokens, but should have them back now. |
| } else if err != nil { | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
any specific reason to make this change from on conditions block to 2 different ones?
There was a problem hiding this comment.
so that the existing err handling code can catch both the "list failed" and "route not admitted" failure modes.
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
openshift-ci-robot
added
the
needs-rebase
wking
force-pushed
the
library-go-route-ingress-uri
branch
from
6ea5e9c
to
ad3f7cd
Compare
openshift-ci-robot
removed
the
needs-rebase
Using the helper from [1], so we: * No longer hard-code https://. It's calculated based on the route's TLS config. * No longer assume that the available ingress host will match the route's spec.host. * No longer assume that the route is admitted. Now we check the ingress conditions to confirm Admitted=True. Generated with: $ emacs cmd/openshift-install/create.go # everything in the diff $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.15.2 linux/amd64 [1]: openshift/library-go#911
We know the name we're looking for, so a Get should be slightly more efficient. The list landed with the route logic in ff53523 (add logs at end of install for kubeadmin, consoleURL, 2018-12-06, openshift#806) without clear motivation. It's possible that the intention was shielding from the console operator renaming their route, with the: Route found in openshift-console namespace: ... lines pointing us at the renamed route. But it seems unlikely that any such renaming would happen, and if it does, we expect the console-operator change to fail its end-to-end testing with the installer not finding a route with the old name.
wking
force-pushed
the
library-go-route-ingress-uri
branch
from
ad3f7cd
to
eb343b1
Compare
|
/approve |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, staebler The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Using the helper from openshift/library-go#911, so we:
https://. It's calculated based on the route's TLS config.spec.host.Admitted=True.Generated with:
using:
The gnostic replacement avoids the casing change that landed in google/gnostic@896953e. We'll want to drop the replacement-pin once we bump k8s.io/client-go/discovery to a version that uses the lowercased package.