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

Use server 127.0.0.1 down entry only when required #1667

Merged
merged 2 commits into from Jun 20, 2021

Conversation

pini-gh
Copy link
Contributor

@pini-gh pini-gh commented Jun 15, 2021

See #1609 (comment).

Additionally, fix a warning due to a missing backslash escaping from previous merge request.

@buchdag buchdag added type/fix PR for a bug fix type/test PR that add missing tests or correct existing tests labels Jun 15, 2021
@buchdag
Copy link
Member

buchdag commented Jun 16, 2021

Do you think the test cases could be expanded to include the following:

  • multiple (three ?) containers with the same VIRTUAL_HOST, one on an unreachable network: assert that we still don't get server 127.0.0.1 down in this setup
  • single container with multiple unreachable networks (I don't know if it's possible): assert that we only get server 127.0.0.1 down once

Or maybe it's overkill ?

Excellent work anyway, thank you.

@pini-gh
Copy link
Contributor Author

pini-gh commented Jun 16, 2021

* multiple (three ?) containers with the same `VIRTUAL_HOST`, one on an unreachable network: assert that we still don't get `server 127.0.0.1 down` in this setup

This implies that they use the same VIRTUAL_PORT, doesn't it?

* single container with multiple unreachable networks (I don't know if it's possible): assert that we only get `server 127.0.0.1 down` once

Will give it a try.

@buchdag
Copy link
Member

buchdag commented Jun 16, 2021

This implies that they use the same VIRTUAL_PORT, doesn't it?

Not necessarily. The idea was to cover the (undocumented, admittedly) use case of people doing load balancing behind nginx-proxy as described in #1654, but failing this test won't mean that this use case would stop working. Both this test and the other one are more unit testing than functional testing, hence the overkill question.

I have no problem merging this now and adding further tests later of you agree that they'd be nice to have.

@pini-gh
Copy link
Contributor Author

pini-gh commented Jun 22, 2021

single container with multiple unreachable networks (I don't know if it's possible): assert that we only get server 127.0.0.1 down once

@buchdag I don't know how to do it using docker-compose v1 syntax. And on my box test_composev2 fails because test_nginx-proxy_1 and test_web_1 are on the auto allocated test_default network while the nginx-proxy-tester instance is on the bridge network. Then the tester can't talk to the usecase instances. Any idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix PR for a bug fix type/test PR that add missing tests or correct existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants