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

Add support for every build context type supported by the daemon #1244

Closed
wants to merge 2 commits into from
Closed

Add support for every build context type supported by the daemon #1244

wants to merge 2 commits into from

Conversation

moysesb
Copy link

@moysesb moysesb commented Apr 2, 2015

This is a Proposal PR.

Implements #1209 (and more) using some of the ideas from docker/docker-py#538

This changeset adds support for specifying the build path in any of the formats supported by docker-py's implementation of the build() operation. I expanded the section on build: in docs/yml.md with a brief explanation of what kind of values are supported. The implementation is mostly in a new file: compose/context.py.

@@ -12,6 +12,7 @@ RUN set -ex; \
curl \
lxc \
iptables \
liblzma-dev \
Copy link
Member

Choose a reason for hiding this comment

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

Nit; indentation looks off

@moysesb
Copy link
Author

moysesb commented Apr 30, 2015

ping @aanand. Any word on whether this is being considered for merging? Thanks!

@aanand
Copy link

aanand commented Apr 30, 2015

Thanks for the PR! I'm theoretically in support of the feature, but:

  • Why are we doing LZMA decompression client-side? Is that how the Docker CLI does it? I'm not relishing the bloat there.
  • A lot of this logic looks like it belongs in docker-py. I see from Design proposal: make the docker-py interface more 'data oriented' docker-py#538 that it's non-trivial to overhaul build and create_container at this time, but the logic in compose.context could at least be moved to docker.utils.
  • I deeply appreciate your including documentation. I don't think it needs to be that comprehensive, though - we should instead make sure the docker build docs cover everything and link to them, and then show a short list of example values for build.

@moysesb
Copy link
Author

moysesb commented Apr 30, 2015

@aanand

  • The lzma thing is just a workaround for the fact that python2's tarfile module doesn't support the lzma format. The rationale is that compose should (perhaps) perform a preliminary sanity check on the archive before submitting it to the daemon, since the upload can be expensive if the file is large. The only way to make sure it's a valid archive with python2 is to include a check using the lzma module. Note that there's no actual decompression on the client side, just a peek to see if the tarfile/lzma module recognizes the archive format.
  • I agree that most of this logic should be in docker-py. I'll wait for the maintainers to review
    Design proposal: make the docker-py interface more 'data oriented' docker-py#538 and then move context.py there.

@moysesb
Copy link
Author

moysesb commented May 7, 2015

@aanand Moved context.py to docker-py and pushed a new commit here. I understand there's a discussion going on about how much validation should be performed by compose for the value of build (in #1372) - the implementation of create_context_from_path() in my docker-py fork does validate the supplied path, so maybe that should be taken into account.

@@ -12,6 +12,7 @@ RUN set -ex; \
curl \
lxc \
iptables \
liblzma-dev \
Copy link

Choose a reason for hiding this comment

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

How widely installed is liblzma? We should make sure it exists on all the main already-supported Linux distros and OS X.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, this is a leftover from when context.py was in compose, there's no actual need for this dependency here. The lzma compression is handled as a special case in docker-py now: under py3 the tarfile module has built-in support for lzma (presuming the xz-utils headers were available when interpreter was built) but under py2 docker-py just accepts a file with an '.xz' extension as a lzma compressed tarball and passes it on to the build call without 'peeking' into it.

Signed-off-by: Moysés Borges <moysesb@gmail.com>
Signed-off-by: Moysés Borges <moysesb@gmail.com>
@moysesb moysesb closed this Sep 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants