Ports --env-file in minimalistic fashion

[vpetersson]

Refs #565. Adds support for providing environment variables from a file instead of manually providing them one-by-one.

[aanand]

Thanks for the contribution!

1. This needs tests, and they should at least cover the cases covered by the Docker client's env file parser tests (although the "too long file" is less important).
2. The Docker client performs very straightforward string splitting. Is there a compelling reason to use csv here, rather than just do the same?

[aanand]

Also, it's not clear what happens when a variable is specified both in a file and in the environment key. Indeed, the developer might want to decide that for themselves (I can say from experience working on Compose that opinions differ on what should take precedence).

Arguably, docker-py should just provide a utility method to parse a dict of environment variables from a file, and leave the job of building the combined dict to the developer.

[vpetersson]

This needs tests, and they should at least cover the cases covered
by the Docker client's env file parser tests (although the "too long file" is less important).

Ok, let me take a look at that.

The Docker client performs very straightforward string splitting. Is there a compelling
reason to use csv here, rather than just do the same?

None really other than I figured that it was better to use a built-in feature instead of re-inventing the wheel (even if a simple .split('=') is likely sufficient).

it's not clear what happens when a variable is specified both in a file and in the environment key.

It works fine:

In [4]: docker.utils.create_container_config('1.18', '_mongodb', 'foo',  environment={'john': 'doe', 'steve': 'smith'}, env_file='/Users/mvip/tmp/foo')['Env']
Out[4]: 
[u'steve=smith',
 u'john=doe',
 'BROKER_HOST=localhost',
 'BROKER_PASSWORD=somePassword',
 'BROKER_PORT=123']

In [5]: docker.utils.create_container_config('1.18', '_mongodb', 'foo',  environment='FOO=BAR', env_file='/Users/mvip/tmp/foo')['Env']                                                                 
Out[5]: 
['FOO=BAR',
 'BROKER_HOST=localhost',
 'BROKER_PASSWORD=somePassword',
 'BROKER_PORT=123']

But you're right, the environment file from disk would override the manual one.

[aanand]

Sorry, I meant: what happens when the same variable (e.g. FOO) is specified in both places with a different value? Which one wins?

I figured that it was better to use a built-in feature instead of re-inventing the wheel

Fair enough - I'm just concerned that it could have all kinds of unexpected extra behaviour. For example, does it have special ways of dealing with quotes, or escaped delimiters? Even if those are useful things, we shouldn't deviate from the Docker client's behaviour.

[vpetersson]

Sorry, I meant: what happens when the same variable (e.g. FOO) is specified in both
places with a different value? Which one wins?

Yeah, in that case the environment file would override. Not sure if this makes sense or not, but it would be easy to add logic if that isn't the expected behavior.

For example, does it have special ways of dealing with quotes, or escaped delimiters?
Even if those are useful things, we shouldn't deviate from the Docker client's behaviour.

Yeah, I was thinking about that too when I wrote it. I could not find any documentation for the escape character (and didn't find the code at first glance). The CSV library does have a feature for explicitly set this though so if you have the escape character, I can easily add that.

[vpetersson]

It looks like the entire 'create_container_config' function lacks testing. I'm not sure if I'm well suited to write a test for this entire function.

[aanand]

The problem is that the "expected behaviour" differs for different people. For example, with the Docker client, values specified with -e take precedence over those specified in environment files, but lots of Compose users would prefer environment files to take precedence over values specified in the environment section of a Compose file.

This is why I think that, as a first step, it'd be preferable to simply provide a utility function for parsing environment files and leave it to the user to construct an environment dict to pass into create_container. That way docker-py isn't dictating the order of precedence.

[vpetersson]

I see your point. Yeah, there are benefits with that, but for the use case I have in mind for this it would actually complicate things further, as you'd then have to re-implement the same stuff there (unless I'm missing something).

[aanand]

You wouldn't be duplicating any logic - the logic for parsing files would live in docker-py, and the logic for building the final environment dict would live in your project.

from docker.utils import parse_env_file

env = #... get env option
env_files = #... get env_file option

for filename in env_files:
    env.update(parse_env_file(filename))

[vpetersson]

@aanand Let me know what you think.

[aanand]

Looks much better, thanks.

1. After skimming the docs for the csv module, I'm convinced we shouldn't use it. There's way too much logic in there for dealing with actual CSV that might result in weird bugs for environment files. It would be best to follow Docker's ParseEnvFile method as closely as possible.
2. The if isinstance(env_file, str) and if os.path.isfile(env_file) checks are unnecessary and will swallow errors that should bubble up to the developer.
3. After that, just needs tests I think.

[vpetersson]

Addressed the feedback, but still working on the tests. I'm having some issues there and have posted it on StackOverflow.

[aanand]

1. If you remove the isfile check, you can remove the first patch from your test.
2. Rather than patching open(), just write the contents to a temporary file (using tempfile) and open() that directly.

[vpetersson]

@aanand Tests added. Let me know what you think.

[aanand]

Thanks!

1. The Docker client raises an error if there are bad lines in the file, so we should do that too.
2. It'd be more useful if parse_env_file returned a dict, so that multiple env files can be combined using the update() method on dict
3. I'm not sure about the change to .gitignore, but that should definitely be done in a separate PR.

[vpetersson]

The Docker client raises an error if there are bad lines in the file, so we should do that too.

Done.

It'd be more useful if parse_env_file returned a dict, so that multiple env files can be
combined using the update() method on dict

Sure. Fixed.

I'm not sure about the change to .gitignore, but that should definitely be done in a separate PR.

Yeah, that was an accidental commit.

[vpetersson]

Also, I'm not sure if I should commit the whole htmlcov folder as well.

[aanand]

Rather than checking twice for line[0] != '#', I think this could be simplified to:

if line[0] == '#':
    continue

parse_line = line.strip().split('=')
if len(parse_line) == 2:
    # ...
else:
    # ...

[vpetersson]

Nice! Yes, that's cleaner. Pushed.

[aanand]

Great stuff. Two last things:

1. Could you add a test for the "invalid line" case?
2. Could you change the error message wording to:

Invalid line in environment file /path/to/file.env:
(offending line goes here)

[vpetersson]

Could you add a test for the "invalid line" case?

There's already a test for that here or what do you mean?

Could you change the error message wording to...

Done.

[aanand]

Great. Needs squashing to one commit, then LGTM.

[vpetersson]

Done like dinner!

[vpetersson]

@aanand can we land this? Would like to start working on the Ansible side of things :)

[aanand]

It's got my LGTM, but another maintainer needs to approve and merge it.

[shin-]

Merged - thanks!