Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[3.7] service catalog: Update API healthz check to use uri module and add noproxy #7283

Merged
merged 2 commits into from Mar 25, 2018
Merged

[3.7] service catalog: Update API healthz check to use uri module and add noproxy #7283

merged 2 commits into from Mar 25, 2018

Conversation

vrutkovs
Copy link
Member

Backport of #7222 and #7262

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 26, 2018
@sdodson
Copy link
Member

sdodson commented Feb 26, 2018

@abutcher I know we discussed this change on Friday, what's your take on whether we move forward with this? If we do I think we also need to go back and change all curl invocations to align with this if possible.

@abutcher
Copy link
Member

@sdodson I was worried we were losing testing TLS here but I see ‘-k’ on the original. URI module seems fine but why are we not validating certs?

@vrutkovs
Copy link
Member Author

why are we not validating certs

Certs might be self-signed, IIUC its required to skip certs on containerized systems at least

@vrutkovs
Copy link
Member Author

/retest

enj and others added 2 commits March 8, 2018 16:47
@enj @vrutkovs
Update API healthz check to use uri module
8e3e295 
This should prevent issues with curl getting upset with certs that have the same serial number and name.

See openshift/origin#18405
@vrutkovs
start_api_server: service catalog healthcheck doesn't require proxy
7ec4aec
@vrutkovs vrutkovs force-pushed the 3.7-service-catalog-no-proxy branch from 5f79ad0 to 7ec4aec Compare March 8, 2018 15:47
michaelgugino
michaelgugino approved these changes Mar 8, 2018
View reviewed changes
Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2018
@vrutkovs
Copy link
Member Author

/retest

4 similar comments
@vrutkovs
Copy link
Member Author

/retest

@vrutkovs
Copy link
Member Author

/retest

@vrutkovs
Copy link
Member Author

/retest

@vrutkovs
Copy link
Member Author

/retest

@openshift-ci-robot
Copy link

@vrutkovs: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install_crio 7ec4aec link /test crio
ci/openshift-jenkins/install 7ec4aec link /test install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@sdodson sdodson merged commit 250151f into openshift:release-3.7 Mar 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelgugino michaelgugino michaelgugino approved these changes

@sdodson sdodson Awaiting requested review from sdodson

@mtnbikenc mtnbikenc Awaiting requested review from mtnbikenc

Assignees

@michaelgugino michaelgugino

Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vrutkovs @sdodson @abutcher @openshift-ci-robot @michaelgugino @enj