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 for environment variables substitution #67

Merged
merged 1 commit into from
Aug 2, 2016

Conversation

surajssd
Copy link
Member

Now user can put environment variables in docker-compose file and it will be read by kompose from environment

Fixes #56

Now user can put environment variables in docker-compose
file and it will be read by kompose from environment

Fixes # 56
@surajssd
Copy link
Member Author

The code in this PR is from https://github.com/docker/libcompose/blob/master/docker/project.go#L25

So I needed some discussion around this PR, so I checked the code in kompose here, it is similar to code in libcompose here. So I was hoping we can use the function NewProject from libcompose which is under docker package not under project package.

So the downside of using docker package's NewProject is it returns project.APIProject and I would like to have a method in project.APIProject that returns internal data structure of type *Project and we don't need to repeat the code. So I will be working with libcompose to get this done. If that works out this PR will change!

@ngtuna
Copy link
Contributor

ngtuna commented Jul 27, 2016

@surajssd I see you are changing libcompose code in /vendor. Is it possible to just work on kompose code base and keep libcompose as a dependency ?

@surajssd
Copy link
Member Author

@ngtuna I did godep save to get latest code from libcompose. Is it not the right way to do it?

@ngtuna
Copy link
Contributor

ngtuna commented Jul 27, 2016

@surajssd Ah okay you changed libcompose revision to HEAD. OK let me check it.

@surajssd
Copy link
Member Author

@ngtuna @kadel How can I convert a APIProject interface object type to Project type? Is there a way to do it?

@ngtuna
Copy link
Contributor

ngtuna commented Jul 27, 2016

@surajssd The reason I didn't use NewProject function because I don't want kompose to be constrained by libcompose. Basically we only need its parsing function in order to parse docker compose file into komposeObject, nothing more. Otherwise, we have to follow libcompose's constraints such as it's required to have a docker-compose.yml file handy at the same folder.

Did you check whether your changes in cli/app/app.go still works with the current libcompose revision at HEAD master? If yes then I suggest to keep the vendoring stable and just apply your changes in app.go. I can't check it now but will do tomorrow morning.

@surajssd
Copy link
Member Author

@ngtuna I checked with the vendored libcompose available I am not able to build, because I need https://github.com/skippbox/kompose/pull/67/files#diff-41d801ef80f1858d5e8e9695667e4dafR749 and it is not in the vendored libcompose

@ngtuna
Copy link
Contributor

ngtuna commented Aug 1, 2016

Checked! Thanks @surajssd . Please go ahead to merge upstream.

@ngtuna ngtuna added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2016
@surajssd
Copy link
Member Author

surajssd commented Aug 2, 2016

@ngtuna thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/discuss lgtm "Looks good to me", indicates that a PR is ready to be merged. review needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants