-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
Please sign your commits following these rules: $ 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. |
cf817ba
to
36094c3
Compare
36094c3
to
6e04f8f
Compare
Rebased to resolve conflict with 9d2508c. |
There was a problem hiding this 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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
728ebef
to
1e73d48
Compare
There was a problem hiding this 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.
e0932a8
to
132bc2d
Compare
Added |
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>
132bc2d
to
413e5db
Compare
Rebased onto master to resolve conflict with #6813. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This covers what was included in docker#6800 Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
This covers what was included in docker#6800 Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
Resolves #6746
The
--env-file
option added in PR #6535 makes sense as a top-level option but was added both toTopLevelCommand
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
, anddown
; moves the handling of the option into a method onTopLevelCommand
; 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:
--env-file
option from thebuild
,up
, anddown
subcommands. You can see that in their usage messages (which you can view by trying to put the option after the subcommand...)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):The output will include
WHEREAMI=override
when it's working. When it's not, i.e. onmaster
, 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 asconfig
seems like it would take a bit of doing and wouldn't test anything very interesting under the circumstances.