restart: no gets interpreted as restart: false

[ibuildthecloud]

The yaml parser seems to turn restart: no into restart: false and
false is not a valid restart policy. This patch manually fixes up the
value to be no if it is read as false.

This is based off of #66 because in that PR I added merge_test.go and didn't want to duplicate it and then have to deal with merge conflicts/rebasing.

[ibuildthecloud]

@dnephin @vdemeester

[dnephin]

I don't think this is the direction we want to go. We've decided against it in docker-compose (at least for now).

Re-writing boolean values to string values is not really intuitive. Could we instead warn the user that they have the wrong type?

I think this will just fix itself when #34 is completed. The jsonschema will raise a validation error if this field is a boolean

[ibuildthecloud]

@dnephin I don't totally follow. The issue is this... If the user does the below

image:
  restart: no

The yaml parser turns restart: no into restart: false. We need to then manually change restart: false back to restart: no.

[dnephin]

No, the yaml parser isn't wrong, the user is wrong.

If you go that route, you end up translating a bunch of things that aren't no into "no".

Using a boolean in a string field should result in an error. The user should write restart: 'no'

[ibuildthecloud]

The user is always right 😄 So seriously you don't want this? This matches the behaviour of docker-compose today. restart: no works in docker-compose without restart: 'no'

[dnephin]

We're changing it to a warning in the next release (1.5.0) now that we're properly validating the config. It'll become an error in the release after that.

I'm not sure how it works, since we shouldn't be doing any conversion of bool to string.

[ibuildthecloud]

@dnephin I don't know how to proceed because a docker-compose.yml from docker compose will work in 1.5 and blow up in libcompose-cli. It sounds to me like we need this change to stay compatible. @joshwget is working on schema validation. Maybe when that PR is done we can revisit this.

[dnephin]

I'm not sure how it works, but I agree we should revisit after validation is in

[vdemeester]

Ah you need to update that I think, to mergeProject.

[ibuildthecloud]

@vdemeester Fix compilation errors with Merge/mergeProject

[vdemeester]

So, we are ok to merge this as is and we'll get back on it when validation hits, right ? (thus we follow docker-compose < 1.5 behavior)

[ibuildthecloud]

@vdemeester I believe that is the agreement. I've created an issue to track this: #72

[dnephin]

I think the reason this doesn't fail validation in compose 1.4.2 is because the check if not restart_config:, so compose actually things there is no config instead of translating it.

I'm not against merging this, but it will become dead code as soon as #34 is implemented

[vdemeester]

Not against either 😉
LGTM 🐸

[glensc]

documentation still suggest unquoted version:

https://docs.docker.com/compose/compose-file/compose-file-v2/#restart

and that results failure on 1.9.0:

# grep restart docker-compose.override.yml
    restart: no

# docker-compose ps
ERROR: The Compose file './docker-compose.override.yml' is invalid because:
services.weblate.restart contains an invalid type, it should be a string

[vdemeester]

@glensc hum this PR is quite old though, we have schema validation now, that should be the same as docker-compose, I would guess it's doing the same with libcompose 👼