dockest icon indicating copy to clipboard operation
dockest copied to clipboard

Allow published binding port to be string

Open celiolatorraca opened this issue 3 years ago • 8 comments

Since docker compose version 2.3.x, the default behavior now is to return the published binding port as string, which breaks the dockest validation.

This PR fixes the issue #300

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

celiolatorraca avatar May 23 '22 16:05 celiolatorraca

Deploy Preview for dockest canceled.

Name Link
Latest commit 58d0b79c2d94bfa056ccd6194e0c11502af265b2
Latest deploy log https://app.netlify.com/sites/dockest/deploys/628bb4acdc9a080008b20c09

netlify[bot] avatar May 23 '22 16:05 netlify[bot]

realmvp

erikengervall avatar May 23 '22 16:05 erikengervall

@erikengervall Should we convert the string ports to number when validating?

I noticed that we expect the ports to be number in a few places afterwards...

celiolatorraca avatar May 23 '22 17:05 celiolatorraca

@erikengervall Should we convert the string ports to number when validating?

I noticed that we expect the ports to be number in a few places afterwards...

Good question...

Kinda want to roll forward and make a note that one has to upgrade docker compose if they wish to use the upcoming version, rather than making it backwards compatible

WDYT?

erikengervall avatar May 23 '22 17:05 erikengervall

@erikengervall Should we convert the string ports to number when validating? I noticed that we expect the ports to be number in a few places afterwards...

Good question...

Kinda want to roll forward and make a note that one has to upgrade docker compose if they wish to use the upcoming version, rather than making it backwards compatible

WDYT?

Well! Sounds also good

The only problem is that we may still need to parse the string to integer at some point. Libs like net require the port to be number and not string

If we choose the "roll forward" approach, we would need just to bump a major version and make it clear somehow that the new version requires docker compose 2.3.x and the older versions should be used if version is lower than that

celiolatorraca avatar May 23 '22 17:05 celiolatorraca

Hello guys, Do you have any updates on this?

emrahyldrm avatar Jul 19 '22 08:07 emrahyldrm

@erikengervall Should we convert the string ports to number when validating? I noticed that we expect the ports to be number in a few places afterwards...

Good question... Kinda want to roll forward and make a note that one has to upgrade docker compose if they wish to use the upcoming version, rather than making it backwards compatible WDYT?

Well! Sounds also good

The only problem is that we may still need to parse the string to integer at some point. Libs like net require the port to be number and not string

If we choose the "roll forward" approach, we would need just to bump a major version and make it clear somehow that the new version requires docker compose 2.3.x and the older versions should be used if version is lower than that

I'm fine with a major bump with this restriction 🙂

Hello guys, Do you have any updates on this?

@emrahyldrm apologies, haven't had the time to sit down and flesh out the code for the above suggestions.


I'd be happy to review a PR containing upgrades to docker compose 2.3.x and do a major bump!

erikengervall avatar Jul 27 '22 20:07 erikengervall

Has there been an official response from the docker team to confirm what the desired behaviour should be?

According to their example in their own docs about specifying ports, the value is a number (even though the docker compose config output is currently giving a string).

If it is a bug on their end, would dockest then have to add another major breaking change if they decide to use numbers again in the docker compose config 😅 ?

I wrote a small proof of concept (before I saw this thread, oops!) that normalizes the published field into a number always. Could this be a viable approach to ship as a non-breaking change? Let me know how I could help, thanks!

rperryng avatar Aug 10 '22 20:08 rperryng