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 pointing to tarball contexts in the 'build' parameter. #1209

Closed
moysesb opened this issue Mar 27, 2015 · 23 comments
Closed

Add support for pointing to tarball contexts in the 'build' parameter. #1209

moysesb opened this issue Mar 27, 2015 · 23 comments

Comments

@moysesb
Copy link

moysesb commented Mar 27, 2015

Since the docker client used by compose (docker-py) supports receiving externally generated tarballs as a parameter for its build() method, this alternative should be available in the project configuration file. i.e.:

service:

build: path/to/tarball.tar.gz

The contents within the compressed file should pass the same level of validation as the currently supported directory paths (for example, 'there must be a Dockerfile at the root inside the tar archive'). Aside from that, the general idea is that the archive is some kind of externally validated artifact, auto-generated by a continuous integration stack, so there should be no need for parsing .dockerignore.

If this sounds like a good idea I can start working on a PR (I already have an working prototype).

@moysesb moysesb changed the title Add support for pointing to tarball contexts on the 'build' parameter. Add support for pointing to tarball contexts in the 'build' parameter. Mar 27, 2015
@dnephin
Copy link

dnephin commented Mar 27, 2015

It seems reasonable to me that build: would support tarballs as well

@aanand
Copy link

aanand commented Mar 27, 2015

Seems like this change belongs in docker-py.

@moysesb
Copy link
Author

moysesb commented Mar 28, 2015

@aanand I think it belongs in both compose and docker-py, actually. It would certainly be possible to just change docker-py so that it automatically detects when the context being passed is already compressed (instead of relying on the combination-of-parameters logic that it uses today). My grip with this approach would be that it involves moving validation and conditional behavior too far down to the library level.

As a principle there's always value in keeping validation and detection of user intent as close as possible to surface code and not deep in libraries. Libraries should provide a set of tailored, focused operations that each do one thing with as little conditional logic as possible, specially with regard to the input parameters (they certainly need a lot of conditional logic to deal with failures on the underlying resource level and such). That arrangement makes it easier both to fail early and to optimize for efficiency where relevant.

So, if docker-py is changed the right way it would not solve this particular problem, because the right way would be to have docker-py offer a more fine grained interface for its build() operation, one call for tarballs, one for Dockerfile, one for directory path - and then have the calling code decide upfront which one to use, instead of having just one interface that knows how to deal with whatever the calling code decides to pass on.

So, either both compose and docker-py could be changed in tandem (the 'right way', probably unrealistic, I don't know) or compose could be changed first, to call docker-py's build method with the right combination of arguments when the build: parameter points to a tarball. That would solve the more visible half of the problem, i.e, the user facing half.

As it's currently implemented, compose implicitly assumes the value of build: to be a directory path, since the value is passed "as is" to docker-py with the combination of parameters that docker-py happens to use to infer a directory path value.

In services.py:

def build(self, no_cache=False):
   log.info('Building %s...' % self.name)
"""
 - Here it should be determined what kind of resource self.options['build'] points to.
  - If it's a tarball, the call to self.client.build should look something like:
    build_output = self.client.build(
        fileobj=open(self.options['build'], 'rb'),
        custom_context=True,
        encoding='gzip',
        tag=self.full_name,
        stream=True,
        rm=True,
        nocache=no_cache,
    )
  - If it's a directory, use the already existing call:
"""
build_output = self.client.build(
    self.options['build'],
    tag=self.full_name,
    stream=True,
    rm=True,
    nocache=no_cache,
)

Then, when docker-py is improved to move away from the combination-of-parameters approach, the implementation in compose could become less crufty than what's necessary now without losing the ability to fail early, etc.

@aanand
Copy link

aanand commented Mar 30, 2015

OK, I'd mistakenly believed the docker client would smartly determine whether the argument passed to it was a directory or a tarball, but it looks like they go different routes (stdin for tarball or single Dockerfile vs. argument for directory).

In that case, I think it's fine for that logic to live in Compose: we should check whether the path passed to build is a directory or a regular file, and in the latter case pass fileobj=open(self.options['build'], 'rb'), as you say.

@dreamcat4
Copy link

@aanand If build command could accept tarball as a regular arg, then it would benefit other compose tools too. When I look in the source code of docker build and it already has the necessary checking functions etc.

We just have to peek the input file, and check if if archive.IsArchive(magic) then set the variable context to be context = ioutil.NopCloser(but). IMHO It does not seem to be too difficult.

Existing tarball handling (the stdin):

https://github.com/docker/docker/blob/master/api/client/build.go#L74-90

Where to add new tarball handling (as regular argument):

https://github.com/docker/docker/blob/master/api/client/build.go#L94-95

Where the context variable gets sent to docker daemon:

https://github.com/docker/docker/blob/master/api/client/build.go#L206-214

@dreamcat4
Copy link

Oh right… I also seem to have forgotten that compose actually doesn't use the CLI client, but directly calls the API instead. (where in the API, tarball is always sent across the same way).

@moysesb
Copy link
Author

moysesb commented Mar 31, 2015

Would it be ok to add backports.lzma as a requirement? Python2's tarfile module doesn't support lzma compression natively. Since the docker daemon accepts xz I think compose should support it too.

@smebberson
Copy link

In two different projects, we have a custom build scripts to achieve a couple of things...

In one project we use tar to build a tarball (concatenating a few directories, and removing a few others) and pass this to docker build. In another project, we remove a directory from the context in which docker build is run so that we don't have to remove this directory as part of the build process (it's a node_modules directory), then run docker build and then move the directory back for development purposes. Having written that, we could use tar in both instances which would be better.

I was going to open a new ticket to discuss support for pre and post build scripts, so that you could alter the build directory as required before and after a build. But I thought I'd chime in here instead... having the ability to point to a tarball would suffice I suppose. The ability to run a pre-build script would be fantastic to generate the tarball, but you could get around this by a custom script that generates a tarball and then executes docker-compose build.

@moysesb
Copy link
Author

moysesb commented Apr 2, 2015

@smebberson I have a working implementation of this feature in the PR referenced above.

My scenario is more or less the same as yours. We have a staging directory with a bunch of maven generated tar archives with Dockerfiles inside them. When someone finishes a project build, the archive is shipped to this staging area. New uploads are detected by an inotify-wait script of some contraption, which then calls docker-compose to rebuild and restart the corresponding image.

@stale
Copy link

stale bot commented Oct 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 10, 2019
@milesrichardson
Copy link

Please don't close

Would love to be able to do something similar to this:

tar -czf - ./srcDirOne ./srcDirTwo ./srcDirThree \
    | docker build -t acme/widget -f widget/Dockerfile -

@stale
Copy link

stale bot commented Oct 15, 2019

This issue has been automatically marked as not stale anymore due to the recent activity.

@stale stale bot removed the stale label Oct 15, 2019
@MichaelKim0407
Copy link

Is this feature getting anywhere? It looks to me that docker-py already supports building from tar:

If you have a tar file for the Docker build context (including a Dockerfile) already, pass a readable file-like object to fileobj and also pass custom_context=True.

@MichaelKim0407
Copy link

Relatively easy implementation (unless I'm missing something). Submitted PR.

@stale
Copy link

stale bot commented Sep 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 8, 2020
@meeseekz
Copy link

can #7283 be merged? this would be very useful.

@stale
Copy link

stale bot commented Sep 14, 2020

This issue has been automatically marked as not stale anymore due to the recent activity.

@stale stale bot removed the stale label Sep 14, 2020
@stale
Copy link

stale bot commented Mar 19, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 19, 2021
@milesrichardson
Copy link

Looks like a small and easy PR, can it be merged?

@stale
Copy link

stale bot commented Mar 20, 2021

This issue has been automatically marked as not stale anymore due to the recent activity.

@stale stale bot removed the stale label Mar 20, 2021
@bevin0708
Copy link

Looking for the same feature.

@stale
Copy link

stale bot commented Oct 30, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 30, 2021
@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically closed because it had not recent activity during the stale period.

@stale stale bot closed this as completed Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants