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

mark explicitly published port as string to avoid user confusion #15375

Closed
wants to merge 1 commit into from
Closed

mark explicitly published port as string to avoid user confusion #15375

wants to merge 1 commit into from

Conversation

glours
Copy link
Contributor

@glours glours commented Aug 12, 2022

Signed-off-by: Guillaume Lours guillaume.lours@docker.com

Proposed changes

As string can be unquoted in Yaml, our example may confused user and let them assume that the ports.published attribut can be an integer.

I explicitly wrapped the value with quotes to remove ambiguity

Related issues (optional)

docker/compose#9306

Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@netlify
Copy link

netlify bot commented Aug 12, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 676a6f5
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62f66d8b824f39000844c7e1
😎 Deploy Preview https://deploy-preview-15375--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 12, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 32d24e8
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62f66db69223dc000745632f
😎 Deploy Preview https://deploy-preview-15375--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@thaJeztah
Copy link
Member

Did that change? Because it definitely was an integer before; if integers are no longer accepted that is a big breaking change; https://github.com/docker/cli/blob/master/cli/compose/schema/data/config_schema_v3.9.json#L226

@jsoriano
Copy link

Did that change?

It changed in docker compose 2.3.0, see docker/compose#9306.

@@ -54,7 +54,7 @@ services:
ports:
- mode: ingress
target: 80

Choose a reason for hiding this comment

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

Is there a reason why target port is a number, but published port is a string?

Copy link
Contributor Author

@glours glours Aug 12, 2022

Choose a reason for hiding this comment

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

because you can use the range syntax to defined published ports as we already mentioned in the compose issue

Choose a reason for hiding this comment

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

Ah ok, target cannot be a range, right? Thanks!

@glours
Copy link
Contributor Author

glours commented Aug 12, 2022

@thaJeztah introduced in compose-go with this PR, new versions of Compose can handle both, the issue comes when an older than v2.3.0 version of Compose try to parse the ports.published port as an integer when it's a string (and docker compose config generate a string by default since version v2.3.0)
So no issue with the CLI, Compose handle that properly

@craig-osterhout craig-osterhout added the area/compose Relates to docker-compose.yml spec or docker-compose binary label Aug 12, 2022
Copy link
Contributor

@dockertopia dockertopia left a comment

Choose a reason for hiding this comment

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

@milas, I have some questions about aligning compose file specification taking this PR into account, could you please take a look at this PR?

@dockertopia dockertopia requested a review from milas August 16, 2022 09:56
@thaJeztah
Copy link
Member

and docker compose config generate a string by default since version v2.3.0)

could config produce an integer (without quotes) if it's only a number (and only fall-back to using a string if it was actually a port range?)

@milas
Copy link
Contributor

milas commented Aug 16, 2022

I'm not totally sure that changing this example will help ease confusion.

I think we should:

  • Clarify in the Compose spec that published can be of type number | string with explanation of port ranges
    • For parsing, implementations MUST support string for both ranges and single ports and MUST support number for single ports
    • For serializing, can be implementation dependent (e.g. always stringifying the published port is acceptable)
  • Update compose-go to use number when single port, and string when range (as suggested by @thaJeztah above)
    • Although current behavior (always stringify) would be compliant based on my spec suggestion, it's causing headache today and is less user-friendly IMO, we should strive for our implementation to go above the bare minimum

I think it's clear from the linked issue that, ignoring compatibility issues, users find the "single port as a string in published while single port as a number in target" behavior undesirable, and given that docker/compose already supports both on input and the spec is vague, aligning things around the least surprising behavior makes the most sense here IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/compose Relates to docker-compose.yml spec or docker-compose binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants