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

[WIP] Add 'skip_port_validation' flag #36

Closed

Conversation

stephenfin
Copy link
Contributor

This allows us to skip port validation for images that publish multiple
ports, not all of which may be active at any given time. We also
introduce reno, which is a release note manager that we can use to
produce a helpful changelog for users.

This is marked WIP since I haven't tested it and it needs unit tests.

This allows us to skip port validation for images that publish multiple
ports, not all of which may be active at any given time. We also
introduce reno, which is a release note manager that we can use to
produce a helpful changelog for users.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: tox-dev#35
@dcrosta
Copy link
Member

dcrosta commented Jun 9, 2019

This looks reasonable, though it is a big hammer that will probably cause a lot of frustration. When I introduced the health check support in #30, I intentionally left the port checking in, even for containers that have a health check. I'm not sure what's the right call, so I opted for the "better safe than sorry" route.

Would it also have worked for your use case (I assume this is in response to #35?) if you could specify specific ports to publish, similar to docker run -p X:Y, and then only port-test that list?

@stephenfin
Copy link
Contributor Author

This looks reasonable, though it is a big hammer that will probably cause a lot of frustration. When I introduced the health check support in #30, I intentionally left the port checking in, even for containers that have a health check. I'm not sure what's the right call, so I opted for the "better safe than sorry" route.

Definitely a big hammer and I'm not sure it's the correct approach (which is why I didn't invest too much time on writing tests yet).

Would it also have worked for your use case (I assume this is in response to #35?) if you could specify specific ports to publish, similar to docker run -p X:Y, and then only port-test that list?

The ability to specify the specific ports that should be published would work ideally actually and I think I'd rather it to this approach. Would we be providing a container-host mapping (X:Y) or could we let the host-side of things be auto-negotiated as happens when you publish all ports (so you'd continue inspecting the environment variables)?

@dcrosta
Copy link
Member

dcrosta commented Jun 10, 2019

We should let docker pick an appropriate port, and discover it the way we do today (from container.attrs). The [docker:whatever] section should get a new list called publish_ports or something like that listing a subset of the EXPOSEd ports.

It probably should be an error if you ask to publish a port the container doesn’t expose, if that’s possible.

@dcrosta
Copy link
Member

dcrosta commented Apr 19, 2020

This is superseded by #49, just merged.

@dcrosta dcrosta closed this Apr 19, 2020
@stephenfin stephenfin deleted the add-skip_port_validation-flag branch April 19, 2020 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants