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.

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

Bug 588 - Run latest build with a default public ip. #626

Merged
merged 2 commits into from Jan 11, 2018

Conversation

shawn-hurley
Copy link
Contributor

Describe what this PR does and why we need it:
This PR will default the PUBLIC_IP to the localhost.

This value will allow for macOS to have this value because it can not find the docker interface docker0.

The issue that used to exist where the service catalog could not contact the broker is no longer a concern

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #588

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 10, 2018
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9f28dae on shawn-hurley:bug-588 into ** on openshift:master**.

@jmrodri jmrodri self-requested a review January 10, 2018 19:54
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

ack

@@ -34,7 +34,7 @@
#

DOCKER_IP="$(ip addr show docker0 | grep -Po 'inet \K[\d.]+')"
PUBLIC_IP=${PUBLIC_IP:-$DOCKER_IP}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there used to be some hope that PUBLIC_IP had already been set. Is that still true? Or is it safe to assume that it is never set until this moment, or at least we are happy always overwriting any prior value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the getting started docs and saw nothing about setting the PUBLIC_IP. Basically, the mac has to have a default that will be set to fix the bug. If there is some better way, checking if PUBLIC_IP is already set, then checking DOCKER_IP and then a defaulting I wouldn't mind doing that. I would need some direction though for that :)

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, ok. I have no idea if it's important, but just noticed that it was a change in behavior. If you're happy with the behavior, I'm happy. :)

That said, IF it was valuable to use a previously-set PUBLIC_IP, I think you could add a line above this one, and revert this line's change:

DOCKER_IP=${DOCKER_IP:-127.0.0.1}

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Changes Unknown when pulling 51307a0 on shawn-hurley:bug-588 into ** on openshift:master**.

Copy link
Member

@mhrivnak mhrivnak left a comment

Choose a reason for hiding this comment

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

LGTM. I'll leave it to you to figure out why travis isn't happy and whether that's relevant.

@shawn-hurley shawn-hurley added the 3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 label Jan 11, 2018
@shawn-hurley shawn-hurley merged commit 3fe05a2 into openshift:master Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 | release-1.1 Kubernetes 1.9 | Openshift 3.9 | Broker release-1.1 needs-review size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting started guide doesn't work on macOS
5 participants