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

Added env-file flag to docker exec #2602

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

BrianWieder
Copy link
Contributor

Signed-off-by: Brian Wieder brian@4wieders.com

This is my first PR for an open source project and I am a college student starting my journey learning GoLang, so I am open to any and all feedback regarding my code, testing, PR, or anything else!

- What I did
Added a flag to bring in environment variables from a file into docker exec.
Closes #1681

- How I did it
Added a flag to the exec command, and if the value is not the empty string, use the existing logic to parse an env file to add them to the slice that contains the environment variables.

- How to verify it
Run docker exec with --env-file flag with a file containing environment variables and print/use any of the variables inside the file in the command.

Ex:
docker exec --env-file .env 123abc sh -c 'echo $ONE`

where .env comprises of:
ONE=1

- Description for the changelog

Added flag in the docker exec command for parsing a file for environment variables and adding them into the environment for the exec.

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2020

Codecov Report

Merging #2602 into master will increase coverage by 0.00%.
The diff coverage is 91.30%.

@@           Coverage Diff           @@
##           master    #2602   +/-   ##
=======================================
  Coverage   58.16%   58.17%           
=======================================
  Files         295      295           
  Lines       21174    21182    +8     
=======================================
+ Hits        12316    12322    +6     
- Misses       7955     7956    +1     
- Partials      903      904    +1     

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! I left comments inline. It became quite some comments, so I also pushed it to a branch if that helps; https://github.com/docker/cli/compare/master..thaJeztah:changes_for_2602?expand=1

Let me know if you want me to open a pull-request against your branch

cli/command/container/exec.go Outdated Show resolved Hide resolved
cli/command/container/exec.go Outdated Show resolved Hide resolved
cli/command/container/exec.go Outdated Show resolved Hide resolved
cli/command/container/exec.go Outdated Show resolved Hide resolved
cli/command/container/exec.go Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
docs/reference/commandline/exec.md Outdated Show resolved Hide resolved
@BrianWieder BrianWieder force-pushed the 1681-docker-exec-env-file branch 3 times, most recently from 27d2820 to a7223af Compare June 28, 2020 23:57
@BrianWieder
Copy link
Contributor Author

BrianWieder commented Jun 28, 2020

@thaJeztah Thank you for the comments and branch with your feedback, I really appreciate them!

I made the changes myself on my branch to reinforce the feedback. I am new to open source, so I apologize if I should've taken a PR against my branch with your changes instead.

I would be happy to make any changes that I may have missed or any further suggestions you may have!

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks!

I made the changes myself on my branch to reinforce the feedback. I am new to open source, so I apologize if I should've taken a PR against my branch with your changes instead.

That's perfect; no need to pull my changes in as a separate commit 👍

I left some more comments on your PR. Also, perhaps you could update the bash and zsh completion scripts to add the new flag?

For zsh:

  1. Add this line below the -e=,--env= line;
    "($help)*"{-e=,--env=}"[Set environment variables]:environment variable: " \
    "($help -i --interactive)"{-i,--interactive}"[Keep stdin open even if not attached]" \
diff --git a/contrib/completion/zsh/_docker b/contrib/completion/zsh/_docker
index 835c8d459..c11342cee 100644
--- a/contrib/completion/zsh/_docker
+++ b/contrib/completion/zsh/_docker
@@ -750,6 +750,7 @@ __docker_container_subcommand() {
                 $opts_attach_exec_run_start \
                 "($help -d --detach)"{-d,--detach}"[Detached mode: leave the container running in the background]" \
                 "($help)*"{-e=,--env=}"[Set environment variables]:environment variable: " \
+                "($help)*--env-file=[Read environment variables from a file]:environment file:_files" \
                 "($help -i --interactive)"{-i,--interactive}"[Keep stdin open even if not attached]" \
                 "($help)--privileged[Give extended Linux capabilities to the command]" \
                 "($help -t --tty)"{-t,--tty}"[Allocate a pseudo-tty]" \

For bash:

  1. Add this "case" to the _docker_container_exec function
		--env-file)
			_filedir
			return
			;;
  1. Update this line to add the env-file option;
    COMPREPLY=( $( compgen -W "--detach -d --detach-keys --env -e --help --interactive -i --privileged -t --tty -u --user --workdir -w" -- "$cur" ) )
			COMPREPLY=( $( compgen -W "--detach -d --detach-keys --env -e --env-file --help --interactive -i --privileged -t --tty -u --user --workdir -w" -- "$cur" ) )

Diff:

diff --git a/contrib/completion/bash/docker b/contrib/completion/bash/docker
index 6a5298319..f86263ec7 100644
--- a/contrib/completion/bash/docker
+++ b/contrib/completion/bash/docker
@@ -1604,6 +1604,10 @@ _docker_container_exec() {
                        __docker_nospace
                        return
                        ;;
+               --env-file)
+                       _filedir
+                       return
+                       ;;
                --user|-u)
                        __docker_complete_user_group
                        return
@@ -1615,7 +1619,7 @@ _docker_container_exec() {

        case "$cur" in
                -*)
-                       COMPREPLY=( $( compgen -W "--detach -d --detach-keys --env -e --help --interactive -i --privileged -t --tty -u --user --workdir -w" -- "$cur" ) )
+                       COMPREPLY=( $( compgen -W "--detach -d --detach-keys --env -e --env-file --help --interactive -i --privileged -t --tty -u --user --workdir -w" -- "$cur" ) )
                        ;;
                *)
                        __docker_complete_containers_running

cli/command/container/exec.go Outdated Show resolved Hide resolved
docs/reference/commandline/exec.md Outdated Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
cli/command/container/exec_test.go Outdated Show resolved Hide resolved
Signed-off-by: Brian Wieder <brian@4wieders.com>
@BrianWieder
Copy link
Contributor Author

@thaJeztah Thanks for the comments! Made some more changes according to your feedback.

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

Completion LGTM, thanks!

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

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, thank you @BrianWieder for this first PR 👏

@silvin-lubecki silvin-lubecki merged commit 1c6dd42 into docker:master Jun 30, 2020
@thaJeztah thaJeztah added this to the 20.03.0 milestone Jun 30, 2020
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.

Add --env-file to docker exec
5 participants