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

default port tcp #99

Closed
wants to merge 2 commits into from
Closed

default port tcp #99

wants to merge 2 commits into from

Conversation

chayim
Copy link

@chayim chayim commented May 20, 2021

closes issue #98

Copy link
Member

@jugmac00 jugmac00 left a comment

Choose a reason for hiding this comment

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

Please have a look at the broken test suite.

tox_docker/__init__.py Show resolved Hide resolved
@chayim chayim requested a review from jugmac00 May 20, 2021 10:17
@jugmac00 jugmac00 requested review from dcrosta and jugmac00 and removed request for jugmac00 May 20, 2021 11:08
@jugmac00
Copy link
Member

I removed myself as a reviewer as I neither use nor maintain this package. I just commented on the broken build to speed up the review process for the core maintainers.

@jugmac00 jugmac00 dismissed their stale review May 20, 2021 11:10

broken build has been fixed

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

We should also add some tests 👍

more on port mapping.
``EXPOSE`` d port to be available on ``HOST_PORT``. In the case where
the protocol is not specified, tox-docker assumes tcp, as per the
dockre default. See below for more on port mapping.
Copy link
Member

Choose a reason for hiding this comment

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

typo here

@dcrosta
Copy link
Member

dcrosta commented May 24, 2021

I'm leaning towards not accepting this -- the inconvenience of having to type "tcp:" seems a small and infrequent price to pay for greater explicitness in the tox-docker config. Does anyone feel really strongly about this?

@chayim
Copy link
Author

chayim commented May 24, 2021 via email

@gaborbernat
Copy link
Member

I'm personally leaning towards @chayim here, but not strongly. Also note the main maintainer here is @dcrosta so ultimately it's up to his decision.

@chayim
Copy link
Author

chayim commented May 27, 2021

We should also add some tests

@gaborbernat Happy to add the tests and finish this off. But if @dcrosta doesn't want this behaviour then we should kill it off.

@chayim
Copy link
Author

chayim commented Jun 22, 2021

Ping @dcrosta do you have a preference - prior to my writing the tests?

@chayim
Copy link
Author

chayim commented Dec 9, 2021

Given the lack of answer - I assume we don't want this. Closing.

@chayim chayim closed this Dec 9, 2021
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.

4 participants