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

Make '--env-file' option top-level only and fix failure with subcommands #6800

Merged
merged 5 commits into from
Jul 25, 2019

Conversation

KlaasH
Copy link

@KlaasH KlaasH commented Jul 12, 2019

Resolves #6746

The --env-file option added in PR #6535 makes sense as a top-level option but was added both to TopLevelCommand and to some subcommands. At best, the handling in the subcommands is redundant and muddies the waters; at worst, it causes bugs, like the fact that, as noted by @digitalist, docker-compose --env-file=custom.env up doesn't work, loading the .env file despite the attempt to override it.

So this removes the option from build, up, and down; moves the handling of the option into a method on TopLevelCommand; and changes the naming a bit to emphasize that this is a top-level option that controls the top-level environment (I've also been thinking of it as "project-level", as opposed to "service-level").

Testing

This does two main things:

  • Removes the --env-file option from the build, up, and down subcommands. You can see that in their usage messages (which you can view by trying to put the option after the subcommand...)
  • Resolves the issue whereby mishandling by some subcommands caused the top-level option to be ignored. For that, you can do something like this, which creates a simple docker-compose.yml and a few env files and runs a command to show that the override is happening (do it in a new directory to avoid clobbering anything important):
cat <<\EOF > docker-compose.yml
version: '3.7'
services:
  test:
    image: busybox
    env_file: .env.conf
    entrypoint: env
EOF

echo 'WHEREAMI=default' > .env
echo "This won't parse as a variable" >> .env
echo 'WHEREAMI' > .env.conf
echo 'DEFAULT_CONF_LOADED=true' >> .env.conf
echo 'WHEREAMI=override' > .env.override

docker-compose --env-file .env.override up

The output will include WHEREAMI=override when it's working. When it's not, i.e. on master, it'll produce an error saying there can't be whitespace in a variable name.

This doesn't add any cases to the test suite because I couldn't really think of good cases to test. PR #6535 added tests for the "working override" case and there were already tests for the "working default" case, and this mostly removes the stuff that's not covered by those. Adding a test that e.g. up works the same as config seems like it would take a bit of doing and wouldn't test anything very interesting under the circumstances.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "revise-env-file-option" git@github.com:KlaasH/compose.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842358785968
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@KlaasH KlaasH force-pushed the revise-env-file-option branch from cf817ba to 36094c3 Compare July 12, 2019 03:58
@KlaasH KlaasH changed the title Revise env file option Make '--env-file' option top-level only and fix failure with subcommands Jul 12, 2019
@KlaasH KlaasH force-pushed the revise-env-file-option branch from 36094c3 to 6e04f8f Compare July 12, 2019 14:30
@KlaasH
Copy link
Author

KlaasH commented Jul 12, 2019

Rebased to resolve conflict with 9d2508c.

Copy link
Collaborator

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

Just a little consideration on the last modification about using the same method

use_cli = not environment.get_boolean('COMPOSE_INTERACTIVE_NO_CLI')
environment_file = toplevel_options.get('--env-file')
toplevel_environment = Environment.from_env_file(project_dir, environment_file)
use_cli = not toplevel_environment.get_boolean('COMPOSE_INTERACTIVE_NO_CLI')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not using use_cli = not self.toplevel_environment.get_boolean('COMPOSE_INTERACTIVE_NO_CLI') here too?

Copy link
Author

Choose a reason for hiding this comment

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

This is a stand-alone function, not a method of TopLevelCommand. Though it seems like it's only called in TopLevelCommand.run, so adding an argument to pass toplevel_environment in could work fine and remove this duplication.

Copy link
Author

Choose a reason for hiding this comment

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

Added a commit to do this--replaced the project_dir argument (which was only used for this) with a toplevel_environment argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool! I didn't have enough context in the browser based diff to see that.

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

We should update the bash completion scripts in this PR too.

@KlaasH KlaasH force-pushed the revise-env-file-option branch from e0932a8 to 132bc2d Compare July 23, 2019 12:35
@KlaasH
Copy link
Author

KlaasH commented Jul 23, 2019

Added --env-file completions for bash, fish, and zsh.

KlaasH added 5 commits July 24, 2019 09:24
Several (but not all) of the subcommands are accepting and processing the
`--env-file` option, but only because they need to look for a specific
value in the environment. The work of applying the override makes more
sense as the domain of TopLevelCommand, and moving it there and removing
the option from the subcommands makes things simpler.

Signed-off-by: Klaas Hoekema <khoekema@azavea.com>
To help prevent confusion between the different meanings and sources
of "environment", rename the method that loads the environment from
the .env or --env-file (i.e. the one that applies at a project level)
to 'toplevel_environment'.

Signed-off-by: Klaas Hoekema <khoekema@azavea.com>
Signed-off-by: Klaas Hoekema <khoekema@azavea.com>
Instead of passing `project_dir` from `TopLevelCommand.run` to
`run_one_off_container` then using it there to load the toplevel
environment (duplicating the logic that `TopLevelCommand.toplevel_environment`
encapsulates), pass the Environment object.

Signed-off-by: Klaas Hoekema <khoekema@azavea.com>
Adds completions for the --env-file toplevel option to the bash, fish,
and zsh completions files.

Signed-off-by: Klaas Hoekema <khoekema@azavea.com>
@KlaasH KlaasH force-pushed the revise-env-file-option branch from 132bc2d to 413e5db Compare July 24, 2019 13:25
@KlaasH
Copy link
Author

KlaasH commented Jul 24, 2019

Rebased onto master to resolve conflict with #6813.

@ulyssessouza ulyssessouza self-requested a review July 25, 2019 08:30
Copy link
Collaborator

@ulyssessouza ulyssessouza left a comment

Choose a reason for hiding this comment

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

LGTM

@ulyssessouza ulyssessouza requested a review from rumpl July 25, 2019 08:30
@rumpl rumpl merged commit 7a7c9ff into docker:master Jul 25, 2019
@KlaasH KlaasH mentioned this pull request Jul 30, 2019
ulyssessouza pushed a commit to ulyssessouza/compose that referenced this pull request Jul 31, 2019
This covers what was included in docker#6800

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
ulyssessouza pushed a commit to ulyssessouza/compose that referenced this pull request Aug 28, 2019
This covers what was included in docker#6800

Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'--env-file' option is ignored
4 participants