Make '--env-file' option top-level only and fix failure with subcommands#6800
Make '--env-file' option top-level only and fix failure with subcommands#6800rumpl merged 5 commits intodocker:masterfrom
Conversation
|
Please sign your commits following these rules: $ git clone -b "revise-env-file-option" [email protected]: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 -fAmending 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. |
ulyssessouza
left a comment
There was a problem hiding this comment.
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.
Why not using use_cli = not self.toplevel_environment.get_boolean('COMPOSE_INTERACTIVE_NO_CLI') here too?
There was a problem hiding this comment.
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.
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.
Cool! I didn't have enough context in the browser based diff to see that.
728ebef to
1e73d48
Compare
rumpl
left a comment
There was a problem hiding this comment.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Klaas Hoekema <[email protected]>
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 <[email protected]>
Adds completions for the --env-file toplevel option to the bash, fish, and zsh completions files. Signed-off-by: Klaas Hoekema <[email protected]>
132bc2d to
413e5db
Compare
|
Rebased onto master to resolve conflict with #6813. |
This covers what was included in docker#6800 Signed-off-by: Ulysses Souza <[email protected]>
This covers what was included in docker#6800 Signed-off-by: Ulysses Souza <[email protected]>
Resolves #6746
The
--env-fileoption added in PR #6535 makes sense as a top-level option but was added both toTopLevelCommandand 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 updoesn't work, loading the.envfile 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-fileoption from thebuild,up, anddownsubcommands. You can see that in their usage messages (which you can view by trying to put the option after the subcommand...)docker-compose.ymland 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=overridewhen 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.
upworks the same asconfigseems like it would take a bit of doing and wouldn't test anything very interesting under the circumstances.