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

Support inline default values for interpolation #3108

Merged
merged 1 commit into from
Oct 5, 2016

Conversation

dnephin
Copy link

@dnephin dnephin commented Mar 10, 2016

Implements part of #2441 to support inline default values.

I have no idea how robust this is, but it's a start.

if named is not None:
if ':-' in named:
var, _, default = named.partition(':-')
return mapping.get(var, default)
Copy link
Author

Choose a reason for hiding this comment

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

These 3 lines are the only real substantial modifications.

I also cut out the arg/kwarg handling and just replaced it with a mapping arg, since that's how we use it.

@aanand
Copy link

aanand commented Mar 15, 2016

Nice. Maybe we should support ${VAR-default} as well, i.e. substitute the default if VAR is unset, but not if it's set to an empty string? That way you can override a variable to the empty string if you want to.

@dreyks
Copy link

dreyks commented Apr 1, 2016

oh, this is just what all of us need =) any hope this will be merged soon?

@marcellodesales
Copy link

@aanand, @dnephin What needs to be done to get this in? Have been waiting for this for a while

@nicooga
Copy link

nicooga commented Sep 19, 2016

What about using environment variables in key names? Like in:

version: '2'

services:
  postgres: {image: 'postgres:9.6'}
  redis: {image: 'redis:3.2.3'}

  web_server:
    build: ../sources/myapp
    depends_on: [postgres]
    env_file: ../env/$ENV.env
    links: [postgres]
    volumes:
      - myapp_${ENV}_source:/source
      - myapp_nginx_socks:/tmp/socks

  sidekiq:
    build: ../sources/myapp
    command: bundle exec sidekiq -e $ENV -c 2 -q default -q carrierwave
    depends_on: [postgres, redis]
    env_file: ../env/$ENV.env
    links: [postgres, redis]
    volumes_from: [web_server]

volumes:
  myapp_${ENV}_db: {external: true}
  myapp_${ENV}_source: {external: true}
  myapp_nginx_socks: {external: true}

For values it seems to be working right, but for keys it throws a syntax error:

ERROR: The Compose file './config/myapp.yml' is invalid because:
volumes value Additional properties are not allowed ('myapp_${ENV}db', 'myapp${ENV}_source' were unexpected)

BTW, is this on latest version? I'm getting variables interpolated with a fresh installed docker-compose.

$ docker-compose -v
docker-compose version 1.8.0, build f3628c7

@dnephin
Copy link
Author

dnephin commented Sep 19, 2016

@marcellodesales I think we're going to try and get this into the next release for the v2.1 file format.

#3653 adds the v2.1 file format, so once that is merged I can rebase this and change it to only apply to the new format.

@dnephin
Copy link
Author

dnephin commented Sep 19, 2016

What about using environment variables in key names?

Variables are not support in keys. Generally I think this is a bad idea, it leads to a lot of extra complexity.

In your case (and in just about every case so far where this has been requested), it is better to use the PROJECT_NAME or -p to vary the names instead of a variable in the key.

The discussion of variables in keys is not really relevant to this PR, so please open a new issue if you feel this is a necessary addition.

@marcellodesales
Copy link

@dnephin Sounds good... I can definitely be up to QA this... I reallllllly need it!

@shin- shin- added this to the 1.9.0 milestone Sep 28, 2016
Copy link

@shin- shin- left a comment

Choose a reason for hiding this comment

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

This needs to be rebased, but code LGTM

@aanand
Copy link

aanand commented Sep 29, 2016

I'd still like to support ${VAR-default} syntax, if it's easy to implement.

@dnephin dnephin force-pushed the inplace_var_defaults branch 3 times, most recently from b00a451 to f1aec1d Compare September 29, 2016 16:24
@dnephin
Copy link
Author

dnephin commented Sep 29, 2016

Ok, rebased, added the ${VAR-default} and gated the feature behind the Compose V2.1 version.

Signed-off-by: Daniel Nephin <[email protected]>
Copy link

@shin- shin- left a comment

Choose a reason for hiding this comment

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

LGTM

@shin- shin- merged commit eac01a7 into docker:master Oct 5, 2016
nVitius added a commit to nVitius/docker.github.io that referenced this pull request Oct 5, 2016
This feature was not supported previously, but
was added in docker/compose#3108
@dnephin dnephin deleted the inplace_var_defaults branch October 5, 2016 21:35
@hadim
Copy link

hadim commented Oct 5, 2016

Nice addition guys !

I am testing it at the moment the master git code and docker-engine 1.12 and it seems to work great except for volumes defaut values :

version: '2.1'
services:

  test:
    container_name: test-name
    image: busybox
    command: "true"
    privileged: true
    volumes:
      - ${DATA_DIR-/home/hadim/test_data_dir}:/data:Z

I get the following error :

ERROR: Invalid interpolation format for "volumes" option in service "test": "${DATA_DIR-/home/hadim/test_data_dir}:/data:Z"

Am I missing something or is that a bug ?

Thanks

@shin-
Copy link

shin- commented Oct 5, 2016

@hadim Looks like a bug - do you mind creating a separate issue for it? Thank you for your help!

@hadim
Copy link

hadim commented Oct 5, 2016

Sure.

nVitius added a commit to nVitius/docker.github.io that referenced this pull request Oct 12, 2016
This feature was not supported previously, but
was added in docker/compose#3108
nVitius added a commit to nVitius/docker.github.io that referenced this pull request Oct 12, 2016
This feature was not supported previously, but
was added in docker/compose#3108
@glensc
Copy link

glensc commented Oct 20, 2016

@dnephin #3108 (comment)

i proposed new syntax for 2.1 here:
#3998 (comment)

that shouldn't add complexity as keys can be still not interpolated and makes syntax more similar to other definitions.

@AlJohri
Copy link

AlJohri commented Aug 9, 2017

@shin- With this config: ports: ["${PORT-8000}:${PORT-8000}"]

I'm getting this error: ERROR: Invalid interpolation format for "ports" option in service "app": "${PORT-8000}:${PORT-8000}"

Am I doing something wrong?

@shin-
Copy link

shin- commented Aug 9, 2017

@AlJohri please share the output of docker-compose version and your entire Compose file.

@tmack8001
Copy link

I also get this same issue where defaults aren't supported... and my docker-compose version is the latest non release candidate, 1.22.0. I'm trying to add an environment variable and default into the "docker_host" of the image.

my-cool-container
    image: ${DOCKER_HOST:-docker.something.internal.com}/namespace/my-cool-image:my-cool-tag

but I get the following error while executing docker-compose up:

ERROR: Invalid interpolation format for "image" option in service "my-cool-container": "${DOCKER_HOST:-docker.something.internal.com}/namespace/my-cool-image:my-cool-tag"

@shin-
Copy link

shin- commented Oct 8, 2018

Additionally when using the 2.1 file format, it is possible to provide inline default values using typical shell syntax

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

Default values only with v2.1 or above.

@tmack8001
Copy link

ah thanks @shin- 👍 I was still referring to v2 🤕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.