Skip to content

Conversation

@vdemeester
Copy link
Collaborator

Environment variable names used by the utilities in the Shell and
Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase
letters, digits, and the '_' (underscore) from the characters
defined in Portable Character Set and do not begin with a
digit. Other characters may be permitted by an implementation;
applications shall tolerate the presence of such names
. Uppercase
and lowercase letters shall retain their unique identities and shall
not be folded together. The name space of environment variable names
containing lowercase letters is reserved for applications.

As it is possible to use them in environment variable, we should also
allow them during interpolation.

It's also required if we want to allow smarter interpolation in the
future (and for docker/app too 👼)

🦁

Signed-off-by: Vincent Demeester vincent@sbr.pm

)

var delimiter = "\\$"
var substitution = "[_a-z][_a-z0-9]*(?::?[-?][^}]*)?"
Copy link
Contributor

@silvin-lubecki silvin-lubecki Jun 15, 2018

Choose a reason for hiding this comment

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

It is only said that do not begin with a digit, so starting with . is valid? 🤔
So the regexp could be: [_a-z.][_a-z0-9.]*(?::?[-?][^}]*)? ?

And by the way a small nit:

var(
    delimiter = ...
    substitution = ...
    patternString = ...
    pattern = ...
)

> Environment variable names used by the utilities in the Shell and
> Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase
> letters, digits, and the '_' (underscore) from the characters
> defined in Portable Character Set and do not begin with a
> digit. **Other characters may be permitted by an implementation;
> applications shall tolerate the presence of such names**. Uppercase
> and lowercase letters shall retain their unique identities and shall
> not be folded together. The name space of environment variable names
> containing lowercase letters is reserved for applications.

As it is possible to use them in environment variable, we should also
allow them during interpolation.

It's also required if we want to allow smarter interpolation in the
future (and for `docker/app` too 👼)

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the allow-dot-in-interpolation-variables branch from 91c9224 to 318812e Compare June 15, 2018 14:25
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

SGTM, but want to check if there's additional rules when using substitution (also wondering if we could somehow share code that's used in the builder (thinking of moby/moby#37134)

@tiborvass
Copy link
Collaborator

@vdemeester sorry if it's obvious, but I don't understand the usecase :S can you give an example of what this would enable?

@thaJeztah
Copy link
Member

So, on a POSIX shell, this is definitely not working.

# export some.odd.var=foo
sh: 1: export: some.odd.var: bad variable name
# echo ${some.odd.var:-bla}
sh: 2: Bad substitution

So I'm a bit on the fence for this one; we removed validation for ENV, because we're delegating that to the container's process; however, in this case we don't delegate, but handle the variable-name ourselves.

Looking for things where this could (potentially) be troublesome, it looks like currently it won't, but did notice this pattern-matching example in the documentation; http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_02_01, looks

# ${parameter%word}
x=file.c
echo ${x%.c}.o
file.o

@vdemeester
Copy link
Collaborator Author

@tiborvass the main use case I can think of is for docker/app, see https://github.com/docker/app/blob/master/examples/voting-app/voting-app.dockerapp/docker-compose.yml#L28 — we have a yaml struct that we use to interpolate.

version: "3.2"
services:
  vote:
    image: ${vote.image.name}:${vote.image.tag}
    ports:
      - ${vote.port}:80
    networks:
      - frontend
    depends_on:
      - redis
    deploy:
      replicas: ${vote.replicas}
      update_config:
        parallelism: 2
      restart_policy:
condition: on-failure

As @thaJeztah said, I'm a bit on the fence too for that (at least for supporting it in docker/cli). I'll try to come up with a better solution, probably an extension of #1128

@tiborvass
Copy link
Collaborator

@vdemeester is there anything preventing from doing ${vote_image_name} ?

@thaJeztah
Copy link
Member

Hm, so wonder if we're conflating env substitution and Go templating; i.e. should app use go-templating ({ .....}), not ${.....})?

@vdemeester
Copy link
Collaborator Author

@tiborvass not really except UX — i.e. the settings file behind a yaml struct, it feels normal to use it as such in the variables (i.e. with the .).

@thaJeztah we definitely are in a way 😝. But the interpolation in the composefile happens to be env substitution like (and only support something close to that now). It doesn't mean it has to support only env substitution.

By doing that PR, I wish(ed) to make composefile used in docker/app also valid in docker/cli (with some smart sauce). It's not an hard requirement so I'll think of something else like #1128 😉.

@thaJeztah
Copy link
Member

it feels normal to use it as such in the variables (i.e. with the .).

If I'm not mistaken, docker/distribution uses underscores as separator for overriding options in the config-file, so that could still be an option.

Could be problematic if the property itself already has an underscore...

@vdemeester
Copy link
Collaborator Author

I'm going to close this one, as https://github.com/docker/cli/pull/1128/files#diff-0b0330d93ba7beed4c771a2bb1b79ce3R42 will allow advanced usage of the template/interpolation and allow to customize the pattern used.

@vdemeester vdemeester closed this Jun 25, 2018
@vdemeester vdemeester deleted the allow-dot-in-interpolation-variables branch June 25, 2018 12:40
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.

5 participants