docs icon indicating copy to clipboard operation
docs copied to clipboard

mark explicitly published port as string to avoid user confusion

Open glours opened this issue 3 years ago • 5 comments

Signed-off-by: Guillaume Lours [email protected]

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)

https://github.com/docker/compose/issues/9306

glours avatar Aug 12 '22 15:08 glours

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
Latest commit 676a6f50973dab3f0d7b74e31698ae33b21d9d2d
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[bot] avatar Aug 12 '22 15:08 netlify[bot]

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
Latest commit 32d24e8ed2fa3f2979f7844437666a0835409db0
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.

netlify[bot] avatar Aug 12 '22 15:08 netlify[bot]

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

thaJeztah avatar Aug 12 '22 15:08 thaJeztah

Did that change?

It changed in docker compose 2.3.0, see https://github.com/docker/compose/issues/9306.

jsoriano avatar Aug 12 '22 15:08 jsoriano

@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

glours avatar Aug 12 '22 16:08 glours

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?)

thaJeztah avatar Aug 16 '22 14:08 thaJeztah

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

milas avatar Aug 16 '22 20:08 milas