/juju

apiserver: listen on localhost in tests #6438

Merged
merged 1 commit into from Oct 12, 2016
+3 −1

Conversation

Reviewers

@voidspace voidspace

@babbageclunk babbageclunk

@mhilton mhilton

@dooferlad dooferlad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@rogpeppe @macgreagoir @jujubot @voidspace @babbageclunk @mhilton @dooferlad
@rogpeppe
Owner

rogpeppe commented Oct 12, 2016

The old newServer code listened on ":0" not localhost,
but that wasn't a problem because nothing relied
on dialing the address taken from the listener.

When the two server-starting pieces of code were consolidated
into one, we didn't change the code to listen on "localhost:0"
instead of ":0", which causes the tests to fail on Windows
where dialing 0.0.0.0 doesn't count as dialing localhost.

Fixes https://bugs.launchpad.net/juju/+bug/1466011.
@mhilton

mhilton approved these changes Oct 12, 2016
View changes

LGTM

@babbageclunk

babbageclunk approved these changes Oct 12, 2016
View changes

Shortest change = best change!

@rogpeppe
apiserver: listen on localhost in tests 
The old newServer code listened on ":0" not localhost,
but that wasn't a problem because nothing relied
on dialing the address taken from the listener.

When the two server-starting pieces of code were consolidated
into one, we didn't change the code to listen on "localhost:0"
instead of ":0", which causes the tests to fail on Windows
where dialing 0.0.0.0 doesn't count as dialing localhost.

Fixes https://bugs.launchpad.net/juju/+bug/1466011.
9388d4e
@rogpeppe
Owner

rogpeppe commented Oct 12, 2016

$$merge$$
@macgreagoir
Contributor

macgreagoir commented Oct 12, 2016

IANAGR, but LGTM :-)
@jujubot
Contributor

jujubot commented Oct 12, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit b20a0ff into juju:master Oct 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment