735 include verb #2337

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@emtq

emtq commented Oct 22, 2013

This patch implements the include verb.

There are tests in buildfile_test.go and buildfile_parser_test.go. All tests pass, both old and new.

Include files must be located in the same directory tree as the main Dockerfile, not in the parent or sibling directories.

@jpetazzo

This comment has been minimized.

Show comment
Hide comment
@jpetazzo

jpetazzo Oct 22, 2013

Contributor

To recap our IRC discussion:

  • I think that this parser is great, and I agree with the fact that it will make future extensions much easier (and more robust as well)
  • my main concern was "how easy/hard is it to add a new command, or change an existing one?"
  • looking at the 2nd commit, I was convinced that it would be easy enough

To make this even better, it would be nice to add some tooling around the generated files; e.g.:

  • add a pre-commit hook to make sure that the files have been properly generated (similar to our gofmt hook)
  • somehow convince git/github to hide the full diff of those files (can we maybe tag them as binary or something like that?)
  • it might also be acceptable to not include the generated files; after all, the Docker build process happens in a Dockerfile, so installing extra tools could be acceptable.

/cc @vieux @creack @crosbymichael

Contributor

jpetazzo commented Oct 22, 2013

To recap our IRC discussion:

  • I think that this parser is great, and I agree with the fact that it will make future extensions much easier (and more robust as well)
  • my main concern was "how easy/hard is it to add a new command, or change an existing one?"
  • looking at the 2nd commit, I was convinced that it would be easy enough

To make this even better, it would be nice to add some tooling around the generated files; e.g.:

  • add a pre-commit hook to make sure that the files have been properly generated (similar to our gofmt hook)
  • somehow convince git/github to hide the full diff of those files (can we maybe tag them as binary or something like that?)
  • it might also be acceptable to not include the generated files; after all, the Docker build process happens in a Dockerfile, so installing extra tools could be acceptable.

/cc @vieux @creack @crosbymichael

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Oct 22, 2013

Member

As long as said tool is cleanly packaged / packageable, I don't have any issues with that from a packager standpoint. Extra kudos if the tool is something that's popular enough to be packaged already. </$0.02>

Member

tianon commented Oct 22, 2013

As long as said tool is cleanly packaged / packageable, I don't have any issues with that from a packager standpoint. Extra kudos if the tool is something that's popular enough to be packaged already. </$0.02>

@emtq

This comment has been minimized.

Show comment
Hide comment
@emtq

emtq Oct 22, 2013

This is the tool: https://github.com/blynn/nex

It builds with 'go build nex.go'.

How should I package it?

emtq commented Oct 22, 2013

This is the tool: https://github.com/blynn/nex

It builds with 'go build nex.go'.

How should I package it?

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Oct 22, 2013

Member

By packaging, I was referring to something that's already in Debian's apt repos, Fedora's yum repos, Gentoo's portage tree, etc. However, being that it's just Go, it seems to me like a very good candidate for the "vendor" directory, and each distro can either bundle or separately package as necessary/desired.

Member

tianon commented Oct 22, 2013

By packaging, I was referring to something that's already in Debian's apt repos, Fedora's yum repos, Gentoo's portage tree, etc. However, being that it's just Go, it seems to me like a very good candidate for the "vendor" directory, and each distro can either bundle or separately package as necessary/desired.

@emtq

This comment has been minimized.

Show comment
Hide comment
@emtq

emtq Oct 22, 2013

All done, please re-check!

buildfile_lexer.go and buildfile_parser.go are now generated during the build process.

emtq commented Oct 22, 2013

All done, please re-check!

buildfile_lexer.go and buildfile_parser.go are now generated during the build process.

@emtq emtq referenced this pull request Oct 22, 2013

Closed

Proper parser for Dockerfile #2266

@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Nov 11, 2013

Collaborator

Hey guys, I apologize for the slow review.

Here is my opinion on an "INCLUDE" instruction, from another similar pull request (#2108):

I am not comfortable merging an "INCLUDE" instruction as described here. It feels like C-style macros:
it doesn't operate at the same level as the rest of the language, so introduces an extra layer of complexity
and can cause side effects. For example if I include a snippet with a "FROM", it may reset the build process,
or cause it to fail.

I agree there is a need to compose builds beyond what the current FROM can do.
I think the long-term solution is to upgrade FROM so that it can point to a source, not just a build image.
That source could be a local directory, or a remote repo (similar to ADD). It would then be built recursively,
and used as a base image.

Thanks guys for taking the time, and sorry for the disappointment.
Always happy to discuss design on #docker-dev or the mailing list.

Collaborator

shykes commented Nov 11, 2013

Hey guys, I apologize for the slow review.

Here is my opinion on an "INCLUDE" instruction, from another similar pull request (#2108):

I am not comfortable merging an "INCLUDE" instruction as described here. It feels like C-style macros:
it doesn't operate at the same level as the rest of the language, so introduces an extra layer of complexity
and can cause side effects. For example if I include a snippet with a "FROM", it may reset the build process,
or cause it to fail.

I agree there is a need to compose builds beyond what the current FROM can do.
I think the long-term solution is to upgrade FROM so that it can point to a source, not just a build image.
That source could be a local directory, or a remote repo (similar to ADD). It would then be built recursively,
and used as a base image.

Thanks guys for taking the time, and sorry for the disappointment.
Always happy to discuss design on #docker-dev or the mailing list.

@shykes shykes closed this Nov 11, 2013

@emtq

This comment has been minimized.

Show comment
Hide comment
@emtq

emtq Nov 11, 2013

Fair enough. What about #2266 which is the bulk of the work?

My concern is not so much with this PR as with getting the foundation PR above reviewed.

emtq commented Nov 11, 2013

Fair enough. What about #2266 which is the bulk of the work?

My concern is not so much with this PR as with getting the foundation PR above reviewed.

@emtq

This comment has been minimized.

Show comment
Hide comment
@emtq

emtq Nov 11, 2013

This is to say that it's fine that this PR does not make it in but #2266 is what makes PRs like this one possible.

emtq commented Nov 11, 2013

This is to say that it's fine that this PR does not make it in but #2266 is what makes PRs like this one possible.

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