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

Support env file #665

Merged
merged 1 commit into from
Dec 9, 2014
Merged

Support env file #665

merged 1 commit into from
Dec 9, 2014

Conversation

benlangfeld
Copy link

Replaces #481 and #479 addressing the issues raised in those tickets.

@AnalogJ
Copy link

AnalogJ commented Nov 23, 2014

I would love to see this merged into fig. I know its only been a few days, but any eta on when that might happen? Environment management is a bit of a pain-point for me right now

@benlangfeld
Copy link
Author

I'd also obviously love to see this merged. Feedback for improvements is welcome and I'll get them done as fast as possible to make this merge-ready.

@cevaris
Copy link

cevaris commented Nov 23, 2014

+1

def _get_environment(self):
env = {}

if 'env_file' in self.options:
Copy link

Choose a reason for hiding this comment

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

I think using self.options here would mean that override_options is missed. I guess this doesn't technically break anything right now, but I don't think the way the -e param is implemented for fig run is correct, and this would make it harder to fix (it should be using override_options instead of setting service.options).

We may also want to add a command line arg for --env-file at some point.

Could you make this take a param of options instead? This would also have a nice side benefit that self would be unused, and the method could be moved off the class to just a module level helper.

@dnephin
Copy link

dnephin commented Nov 23, 2014

Thanks for the PR! I think this is looking good, I just left a few suggestions. A git squash on some of the commits would be good too, once it's all ready.

@benlangfeld
Copy link
Author

I'll address your suggestions tomorrow and then request another round of review :)

@benlangfeld
Copy link
Author

I think I've now resolved all of your suggestions. I'll leave the commits separate for one more round of review, then squash for merge later.

@mbleigh
Copy link

mbleigh commented Nov 29, 2014

👍 this would be great

@Jasperswaagman
Copy link

+1

@dnephin
Copy link

dnephin commented Dec 2, 2014

LGTM (with a squash)

@marksteve
Copy link

🎉

@benlangfeld
Copy link
Author

Rebased as requested. Merge-ready.

@dnephin dnephin added this to the 1.1.0 milestone Dec 3, 2014
@dnephin
Copy link

dnephin commented Dec 3, 2014

hmm, looks like the latest test run hit some errors, not sure if that was a problem on werkers end or what happened.

I've put this into the milestone for the next release.

@benlangfeld benlangfeld force-pushed the support-env-file branch 2 times, most recently from 894dec9 to 8ffdbbe Compare December 3, 2014 13:39
@benlangfeld
Copy link
Author

Now this is ready :)

@benlangfeld
Copy link
Author

@dnephin Could we get this merged? It's being discussed as a failing of Fig over at moby/moby#9459 (comment).

@dnephin
Copy link

dnephin commented Dec 5, 2014

We need another LGTM from a fig maintainer. I believe they are busy with dockercon EU, so it may be a week or so.

@aanand @bfirsh when you have a minute

@@ -120,6 +120,21 @@ environment:
- SESSION_SECRET
```

### env_file

Add environment variables from a file. You must use an array.
Copy link

Choose a reason for hiding this comment

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

I wonder if we could support the normal use case where it isn't an array? Should just be an isinstance(..., list) right?

Copy link
Author

Choose a reason for hiding this comment

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

Done :)

@bfirsh
Copy link

bfirsh commented Dec 8, 2014

Looks awesome, thanks @benlangfeld! Needs a rebase though.

@benlangfeld benlangfeld force-pushed the support-env-file branch 2 times, most recently from 068b617 to 2fd5df9 Compare December 8, 2014 23:29
@benlangfeld
Copy link
Author

Rebased and ready for merge :)

Signed-off-by: Ben Langfeld <ben@langfeld.me>
@bfirsh
Copy link

bfirsh commented Dec 9, 2014

LGTM

bfirsh added a commit that referenced this pull request Dec 9, 2014
@bfirsh bfirsh merged commit e794e79 into docker:master Dec 9, 2014
@bfirsh
Copy link

bfirsh commented Dec 9, 2014

Thanks @benlangfeld!

@marksteve
Copy link

🎉

@eudaimos
Copy link

Looking at the code, it appears as if env_file is only an option from fig.yml, is this correct?

I've been using the --env-file option to docker run to choose an environment file for "dev", "qa", "prod." Is it possible to extend this as an option to fig up and fig run which would make it act exactly like it does for docker run?

@benlangfeld
Copy link
Author

You are correct @eudaimos. It would be possible to add this as a command-line override as suggested by @dnephin here. Do you think you could add this?

@benlangfeld benlangfeld deleted the support-env-file branch December 14, 2014 17:49
@eudaimos
Copy link

Ha, didn't see that part of the earlier comment. I can give it a try when I free up, but it would be hacky at best and take me a while to get to testing since I've never written anything in python before. @benlangfeld if there's someone else with python experience that wants to take it on before all of that happens, I'd be grateful.

@benlangfeld
Copy link
Author

I'm in precisely the same position as you @eudaimos. This is the first Python code I've ever written :)

@geowa4
Copy link

geowa4 commented Dec 20, 2014

Any idea when this will be delivered?

@abesto
Copy link

abesto commented Jan 2, 2015

Looking forward to this a lot. Any chance of a minor release soon?

@johanhaleby
Copy link

I'd also be really happy for this. This is a showstopper.

@petebrowne
Copy link

👍 Our team is waiting for this one as well.

@Starefossen
Copy link

+1 we are waiting on this too!

@dnephin
Copy link

dnephin commented Feb 15, 2015

This feature is available in the latest docker-compose RC releases https://github.com/docker/fig/releases

yuval-k pushed a commit to yuval-k/compose that referenced this pull request Apr 10, 2015
Support env file
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
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.