Skip to content

Add support for build arguments#2540

Closed
garrettheel wants to merge 1 commit into
docker:masterfrom
Runscope:build-args
Closed

Add support for build arguments#2540
garrettheel wants to merge 1 commit into
docker:masterfrom
Runscope:build-args

Conversation

@garrettheel
Copy link
Copy Markdown

Fixes # #2163

@dnephin
Copy link
Copy Markdown

dnephin commented Dec 12, 2015

Thanks for the PR! Please see the proposed format in #2163 for how we'd like to implement this,

@garrettheel
Copy link
Copy Markdown
Author

@dnephin Ah - I see, that makes sense. Is this something that someone @ dockerhq is implementing or should I tackle that issue as a whole?

@dnephin
Copy link
Copy Markdown

dnephin commented Dec 13, 2015

@garrettheel no one is working on it yet, so if you'd like to pick it up, that would be great

Let me know if you have any questions, either here or on irc

@garrettheel
Copy link
Copy Markdown
Author

@dnephin I had a go at implementing this, could you take a look?

@aanand
Copy link
Copy Markdown

aanand commented Dec 14, 2015

Looks like a great start. I think it'd be better to only support build as an object in the new version of the file format, however.

We should probably extract the config versioning stuff out of #2421 and get it into master so it's not blocking PRs like this one.

Comment thread compose/config/config.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please use six.string_types instead of this try/except

@dnephin
Copy link
Copy Markdown

dnephin commented Dec 14, 2015

Thanks! this is looking great. I think we'll need a couple days to get #2421 merged so that we have version support and we can add this to the v2 config.

@garrettheel
Copy link
Copy Markdown
Author

Fixed all of your comments. I'll keep an eye on #2421 and rebase when it's merged

@vdemeester
Copy link
Copy Markdown

/ping @garrettheel #2421 got merged 😉

@garrettheel
Copy link
Copy Markdown
Author

@dnephin @aanand did we still want to support the build: . shortcut for version 2 or always expect a dict?

@dnephin
Copy link
Copy Markdown

dnephin commented Jan 11, 2016

I like the idea of supporting build: . for the case when you don't have any other build related settings. As soon as you need args, or dockerfile, you need to use the dict.

@c4tz
Copy link
Copy Markdown

c4tz commented Jan 12, 2016

👍 @dnephin, but that one has to be well documented, I think. Otherwise, people will probably get a headache trying to use both build: . and the dict for the settings at the same time ;)

Also: In which version and when will this (presumably) be released? I really need it! :)

@dnephin dnephin added this to the 1.6.0 milestone Jan 12, 2016
@dnephin
Copy link
Copy Markdown

dnephin commented Jan 12, 2016

I've added the version milestone (1.6)

@marcellodesales
Copy link
Copy Markdown

@dnephin Is there anything people can help to get this built? I've been waiting on this to see #2111... I'd like to build from source and test...

Also, great to see it will make finally the cut for 1.6.0

@dnephin
Copy link
Copy Markdown

dnephin commented Jan 12, 2016

We've made some rather large changes to support the v2 format, and I think this branch and #2458 will cause a few merge conflicts with each other.

Either way, I think you can expect to see something in master by the end of the week, but until then there isn't much you'll be able to do.

@garrettheel
Copy link
Copy Markdown
Author

I've rebased this off #2421 and #2458 now.

The documentation could use some work, especially between versions. I.e dockerfile being moved into the the build object in version 2.

Let me know what you think.

@marcellodesales
Copy link
Copy Markdown

@dnephin I will have some time tomorrow to build and run this local... I really need this feature! 👍 Thanks!

@dnephin
Copy link
Copy Markdown

dnephin commented Jan 14, 2016

Needs another rebase.

@garrettheel just want to let you know that we've got code freeze coming up soon, and we want to get this in. We may need to carry this PR and make some small changes to get it merged. #2653 is rebased on the latest master with a few small changes.

I'll leave a few comments here about the docs changes, but we might just end up pulling them into #2653 if we run out of time.

Comment thread compose/config/config.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think since parse_build_arguments() always returns a new dict, these two lines could be:

args = parse_build_arguments(build.get('args'))

Comment thread docs/compose-file.md
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We have #2622 to finalize this, but I think what we'd like to do is only include the v2 docs in this section, and have a separate section at the bottom for migration, and differences between the versions.

Allows 'build' configuration option to be specified as an
object and adds support for build args.

Signed-off-by: Garrett Heel <garrettheel@gmail.com>
@garrettheel
Copy link
Copy Markdown
Author

I've rebased this again off master

@shin-
Copy link
Copy Markdown

shin- commented Jan 14, 2016

Closed via #2653

@shin- shin- closed this Jan 14, 2016
@dnephin
Copy link
Copy Markdown

dnephin commented Jan 14, 2016

@garrettheel thanks again for all your work getting this in!

@marcellodesales
Copy link
Copy Markdown

@garrettheel Thanks all!!! So waited feature!

@garrettheel garrettheel deleted the build-args branch January 15, 2016 04:53
@gggeek
Copy link
Copy Markdown

gggeek commented Jan 16, 2016

cant wait for compose 1.6 to have this!

@marcellodesales
Copy link
Copy Markdown

@gggeek The most complete docker-compose yet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants