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

Add support to pass env-file to docker compose run #9169

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KoditkarVedant
Copy link
Contributor

What I did
This pull request adds support to pass in --env-file to docker compose run command.

Behavior of the option:
If you pass env-file from CLI it overrides the env-files provided in docker-compose.yaml config.

Related issue
#8379

(not mandatory) A picture of a cute animal, if possible in relation with what you did
image

@KoditkarVedant KoditkarVedant force-pushed the 8379-support-env-file-in-docker-compose-run-command branch from 25c370a to 5a1f2bd Compare February 13, 2022 20:39
@KoditkarVedant KoditkarVedant marked this pull request as ready for review February 26, 2022 04:39
@ndeloof
Copy link
Contributor

ndeloof commented Mar 1, 2022

command documentation also need to be updated to reflect this new flag: https://github.com/docker/compose/blob/v2/docs/reference/docker_compose_run.yaml

@KoditkarVedant
Copy link
Contributor Author

@ndeloof Looks like there is bug in the implementation. It isn't working as expected. I think when we parse the compose config files at that time only the env_file mentioned in it gets parsed. because when I see the service.Environment it was already populated with the key=value mentioned in the env_file. So my understanding that just replacing the service.EnvFile will work is not true.

I'll just put back this PR in draft as it is not ready for merge.

@KoditkarVedant KoditkarVedant marked this pull request as draft March 2, 2022 16:16
@ndeloof
Copy link
Contributor

ndeloof commented Mar 11, 2022

The main issue I see adding support for compose run --env-file ... to set container environment from a file (which would make sense to align with docker run ...) is that this will introduce confusion with the top-level --env-file used to override .env file used to interpolate variables. i.e.
docker compose --env-file xx run is a distinct thing vs docker compose run --env-file xx
I'm not sure how we could address this UX issue. Any thoughts @glours @ulyssessouza @thaJeztah ?

@ulyssessouza
Copy link
Collaborator

Same for me... The UX doesn't help... And specially because the new one is an array and the root command one is a single file.

@@ -141,6 +142,7 @@ func runCommand(p *projectOptions, backend api.Service) *cobra.Command {
flags := cmd.Flags()
flags.BoolVarP(&opts.Detach, "detach", "d", false, "Run container in background and print container ID")
flags.StringArrayVarP(&opts.environment, "env", "e", []string{}, "Set environment variables")
flags.StringArrayVar(&opts.EnvFiles, "env-file", []string{}, "Read in a file of environment variables")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we use --service-env-file to avoid ambiguity with upper-level compose command --env-file flag

Copy link
Member

Choose a reason for hiding this comment

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

If we want to retain a parallel between docker run and docker compose run, then I'm wondering if we should look at doing the reverse, and think of a new name for the flag for setting the .env (we could keep the old flag, but hidden)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed the option to service-env-file

@KoditkarVedant KoditkarVedant requested review from ndeloof and thaJeztah and removed request for ndeloof January 24, 2023 17:40
@KoditkarVedant KoditkarVedant marked this pull request as ready for review January 24, 2023 17:41
@KoditkarVedant KoditkarVedant force-pushed the 8379-support-env-file-in-docker-compose-run-command branch from a877dc5 to 5b7b2a3 Compare January 24, 2023 17:50
@KoditkarVedant KoditkarVedant force-pushed the 8379-support-env-file-in-docker-compose-run-command branch from 5b7b2a3 to bfb8c20 Compare January 24, 2023 17:51
Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM but path to service env file passed by command line flag could be relative to working directory

@@ -127,4 +127,8 @@ func applyRunOptions(project *types.Project, service *types.ServiceConfig, opts
for k, v := range opts.Labels {
service.Labels = service.Labels.Add(k, v)
}

if opts.ServiceEnvFiles != nil {
service.EnvFile = append(service.EnvFile, opts.ServiceEnvFiles...)
Copy link
Contributor

Choose a reason for hiding this comment

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

need to make service envfile an absolute path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there any sample code which I can refer in the repository?

Copy link
Contributor

@ndeloof ndeloof May 10, 2023

Choose a reason for hiding this comment

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

You can just use:

if !filepath.IsAbs(file) {
	file, err = filepath.Abs(file)
}

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Base: 73.89% // Head: 73.89% // No change to project coverage 👍

Coverage data is based on head (bfb8c20) compared to base (d47f0f3).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##               v2    #9169   +/-   ##
=======================================
  Coverage   73.89%   73.89%           
=======================================
  Files           2        2           
  Lines         272      272           
=======================================
  Hits          201      201           
  Misses         60       60           
  Partials       11       11           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -156,6 +157,7 @@ func runCommand(p *ProjectOptions, streams api.Streams, backend api.Service) *co
flags := cmd.Flags()
flags.BoolVarP(&opts.Detach, "detach", "d", false, "Run container in background and print container ID")
flags.StringArrayVarP(&opts.environment, "env", "e", []string{}, "Set environment variables")
flags.StringArrayVar(&opts.ServiceEnvFiles, "service-env-file", []string{}, "Read in a file of environment variables")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: rename flag --env-from-file, wdyt ?

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.

4 participants