-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Make '--env-file' option top-level only and fix failure with subcommands #6800
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
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 -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.
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
rumpl
left a comment
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. |
ulyssessouza
left a comment
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-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.