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 --skip-env-file option #6850

Conversation

hirochachacha
Copy link

Fixes #6741
Updates #6511

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "feature/add_skip_env_file_option" [email protected]:hirochachacha/compose.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Fixes docker#6741
Updates docker#6511

Signed-off-by: Hiroshi Ioka <[email protected]>
@hirochachacha hirochachacha force-pushed the feature/add_skip_env_file_option branch from b70e7ae to 04ce00a Compare August 14, 2019 11:12
@jonmchan
Copy link

Though this PR is a step in the right direction. I don't think it addresses the fundamental need that some projects need docker to not evaluate .env at all. Forcing devs to always type --skip-env-file as part of their regular workflow does not properly address the issue imho.

@alecgibson
Copy link

I'm not really sure this fixes #6741. It's not really that different to having a separate (empty) Docker .env file and using the existing --env-file argument.

I think the whole point of the issue is that Docker is being a bit too over-eager with its defaults (ie I'd expect the default to not read anything, and if we have a .env file, that we'd have to specify which one to read). If you do want to do automagical auto-reading, I'd expect it to only read an application-specific file (in the same way you might have a .gitignore, .eslintrc, .bablerc, etc.).

Obviously that's all a breaking change, so I'm not really sure what to suggest, apart from the original suggestion in the issue where we can specify .env from the docker-compose.yml file. Perhaps another suggestion might be to attempt to parse the .env file, and only WARN if it's not valid (because then it probably wasn't meant for Docker consumption?)

@hirochachacha
Copy link
Author

Hi, @jonmchan @alecgibson

Thank you for your comments.

I don't think it addresses the fundamental need that some projects need docker to not evaluate .env at all. Forcing devs to always type --skip-env-file as part of their regular workflow does not properly address the issue imho.

It's true that this CL doesn't implement the yaml part, but I have a reason. Let me explain.

Options:
  -f, --file FILE             Specify an alternate compose file
                              (default: docker-compose.yml)
  -p, --project-name NAME     Specify an alternate project name
                              (default: directory name)
  --verbose                   Show more output
  --log-level LEVEL           Set log level (DEBUG, INFO, WARNING, ERROR, CRITICAL)
  --no-ansi                   Do not print ANSI control characters
  -v, --version               Print version and exit
  -H, --host HOST             Daemon socket to connect to

  --tls                       Use TLS; implied by --tlsverify
  --tlscacert CA_PATH         Trust certs signed only by this CA
  --tlscert CLIENT_CERT_PATH  Path to TLS certificate file
  --tlskey TLS_KEY_PATH       Path to TLS key file
  --tlsverify                 Use TLS and verify the remote
  --skip-hostname-check       Don't check the daemon's hostname against the
                              name specified in the client certificate
  --project-directory PATH    Specify an alternate working directory
                              (default: the path of the Compose file)
  --compatibility             If set, Compose will attempt to convert keys
                              in v3 files to their non-Swarm equivalent
  --env-file PATH             Specify an alternate environment file
  --skip-env-file             Skip loading an environment file

This is an excerpt from the output of docker-compose --help.
You'll probably notice that your point can be applied almost all options aside from --skip-env-file.
In other words, we actually have two different problems.

  1. There is no options to disable .env loading.
  2. Putting a command with long options isn't user friendly.

I'd say it's better to focus the problem 1 on the issue, and we could make another issue or proposal for the problem 2. The problem 2 is general problem, which presumably require widely discussions.

It's not really that different to having a separate (empty) Docker .env file and using the existing --env-file argument.

The result is the same, but I think it's more understandable. At least, you don't have to create empty .env, also your colleagues don't have check the .env.

I think the whole point of the issue is that Docker is being a bit too over-eager with its defaults (ie I'd expect the default to not read anything, and if we have a .env file, that we'd have to specify which one to read). If you do want to do automagical auto-reading, I'd expect it to only read an application-specific file (in the same way you might have a .gitignore, .eslintrc, .bablerc, etc.).

Maybe, but I don't think it's going to happen soon. I assume it requires a kind of major version upgrade.

Perhaps another suggestion might be to attempt to parse the .env file, and only WARN if it's not valid (because then it probably wasn't meant for Docker consumption?)

Yes, it's a possible solution. I'd like to note that it can coexist with this one. I'd say it's nice there is a solution without WARN.

@jonmchan
Copy link

jonmchan commented Sep 1, 2019

I did start looking into checking docker-compose.yml for a skip-env-file flag, but I found that the 1st thing the docker-compose does is read the .env file before reading in the docker-compose.yml file. This makes a lot of sense since the .yml file uses variable substitution that can be taken from the .env file. If we were to implement a flag in the docker-compose.yml file, it would have to read the file 2x. Wasn't sure how favorable the maintainers would be to such a solution which is why I never put in a PR.

I think though that reading from the docker-compose.yml file is the only long term solution that meets all the requirements. For users that use .env for other programs, they would want a way to permanently disable docker-compose from using the file. A command line option would mean this command would have to be typed for every invocation every time (imagine how inconvenient - docker-compose up --skip-env-file, docker-compose down --skip-env-file, docker-compose build --skip-env-file) typing the flag over and over again doesn't seem like the proper solution. Thoughts?

@jonmchan
Copy link

jonmchan commented Sep 2, 2019

@hirochachacha btw, i want to let you know i did read your reply and i acknowledge your approach and what you're trying to do; the point I am trying to reiterate is that long term, it doesn't make sense for a project that wants to ignore .env file to use any command line option at all - short or long option. Having the project set to never use .env in something like docker-compose.yml makes the most sense to me.

@esvm
Copy link

esvm commented Sep 4, 2019

Some news about this?

@Quintasan
Copy link

Can we not merge this and revert to docker-compose ignoring lines from .env it can't understand? I see no reason for this to be merged. The only thing it does is breaking workflows for people.

@esvm
Copy link

esvm commented Sep 9, 2019

Can we not merge this and revert to docker-compose ignoring lines from .env it can't understand? I see no reason for this to be merged. The only thing it does is breaking workflows for people.

Actually we have a lot of people with problems with the support of .env, but we also have a lot of people enjoying this feature. Remove this feature isn't a good idea because it will cause more problems with the people that are already using this. I think that add an option in CLI is actually the best solution we have. And it needs to be merged as soon as possible, is really really so boring downgrade the compose version every time or exclude my .env files and add after

@Quintasan
Copy link

@esvm I fail to understand why ignoring lines that do not conform to VARIABLE=VALUE would be problematic for anyone.

@hirochachacha
Copy link
Author

Previous behavior was a bug, that was out of spec. If you try to revert that behavior on purpose, you don't just solve your problem, but also make that behavior be in spec. That strange spec could be problematic for people in future. Changing the spec just for working around your issue? I'm not sure that whether you deserve it or not.

@hirochachacha
Copy link
Author

hirochachacha commented Sep 9, 2019

Btw, I found that pipenv uses the clever approach.

They're using environmental variable instead of CLI parameters.
It maybe satisfy some people's requirements.
Once, you add COMPOSE_SKIP_ENV_FILE=1 to your .bashrc , you can forget everything.

Signed-off-by: Hiroshi Ioka <[email protected]>
@hirochachacha hirochachacha force-pushed the feature/add_skip_env_file_option branch from 61e62db to cd9ddb4 Compare September 10, 2019 00:45
@Quintasan
Copy link

Quintasan commented Sep 10, 2019

Previous behavior was a bug, that was out of spec. If you try to revert that behavior on purpose, you don't just solve your problem, but also make that behavior be in spec. That strange spec could be problematic for people in future. Changing the spec just for working around your issue? I'm not sure that whether you deserve it or not.

Yes, VAR=VAL syntax appears to be part of the spec. However the same spec does NOT say what will happen to lines that are not compliant. Hence my request to make docker-compose ignore them. This way you get what you wanted and most of people reporting the issues get to keep their .env files and deploy schemas. All you'd have to do is adjust the spec and revert to the previous behaviour. I don't think that skipping the lines that don't conform to the schema docker-compose prefers is that big of a cost. It's not that rare in the software development industry for the spec to change, is it? Should the previous behaviour present a huge obstacle in the future then you can modify the spec once again and change the behaviour once again. But at this point in time I believe this creates more problem than it solves.

I'm hardly the only person that has problems with docker-compose fixing this bug. Please note that docker-compose is not the only application in the entire world that is using this file as mentioned here.

And to address the last part of your reply - I don't deserve anything. I don't think any person in the thread reporting the issue deserves anything. You are free to run your project in any way you want. Since you decided to let your users raise issues with the software I'm free to raise the issue of docker-compose breaking my workflow in many application (which, I unfortunately can't show you since they are not my IP) and to politely disagree with you. If you felt offended by any of my statements then I'd like to apologize - it was not my intention to offend anyone.

@jonmchan
Copy link

@hirochachacha - using pipenv's approach sounds like a favorable compromise. Although users won't be able to have project specific settings, I suspect that most people will not be working with multiple projects that utilize docker env and other projects that do not on the same system.

@hirochachacha
Copy link
Author

hirochachacha commented Sep 13, 2019

@Quintasan I think you should understand the fact this is a very minor issue, even though that's contrary to your intuition and your observation. I guess that's the reason this issue is still open. That's why "de facto standard" approach doesn't make sense to me.

@jonmchan Thank you for your feedback. If users want project specific settings, they could use other tools like direnv, so I hope this is good enough approach.

@Quintasan
Copy link

Quintasan commented Sep 13, 2019

@hirochachacha Okay, let me put things together:

  1. This issue is very minor.
  2. The spec is incomplete with regards to expected behaviour when encountering "incompatible" lines. This is true no matter how you spin it unless the spec is somewhere else.
  3. You refuse to change the spec to state the desired behaviour when encountering incompatible lines.
  4. You are adamant to implement the breaking change despite a better option being present.

If this issue is minor (as you said it) then why the adamant refusal to adopt the behaviour that would result in the smallest amount of breakage and would still give you the result you wanted?

This hardly makes any sense.

@hirochachacha
Copy link
Author

Actually, I'm fine with any solutions. To be honest, I'm not much interested in this issue.
I just said I don't think your option is better than this one.
However if you believe that your options is the best, then go ahead.
I'm not a stakeholder. You can try to convince docker-compose team instead of me.
Anyway, don't talk to me anymore.

@CyrilHu
Copy link

CyrilHu commented Nov 8, 2019

Some news about this?

@hirochachacha
Copy link
Author

It looks like docker-compose team made a great decision. #7150
Since it changes some semantics, perhaps it comes with major version up?

@willsheppard
Copy link

@jonmchan maybe reading the docker-compose.yml file twice would actually be okay.

@Daghall
Copy link

Daghall commented Jan 31, 2022

Workaround: alias docker-compose='docker-compose --env-file /dev/null'

@glours
Copy link
Contributor

glours commented Jul 13, 2022

Thanks for taking the time to create this issue/pull request!

Unfortunately, Docker Compose V1 has reached end-of-life and we are not accepting any more changes (except for security issues). Please try and reproduce your issue with Compose V2 or rewrite your pull request to be based on the v2 branch and create a new issue or PR with the relevant Compose V2 information.

@glours glours closed this Jul 13, 2022
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.

Feature Request: option to disable reading from .env