-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Support alternate Dockerfile name. #1075
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
Conversation
compose/service.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right, fileobj would be for the whole context. This needs to use the dockerfile kwarg.
https://github.com/docker/docker-py/blob/master/docker/client.py#L301
I don't think that option is in a release yet. I added it after the 1.0.0 release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the version I had locally had the following, so I'm assuming it hasn't been released yet
def build(self, path=None, tag=None, quiet=False, fileobj=None,
nocache=False, rm=False, stream=False, timeout=None,
custom_context=False, encoding=None, pull=True,
forcerm=False):This was the only way I could figure out how to get it to work (with current release), dockerfile param would be much better once it's released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think I should update this to use dockerfile (even know it's not released yet?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great. I think we can ask for a new release when this is ready.
I believe fileobj doesn't actually work when the Dockerfile makes use of other files in the build context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I was just testing that, and you are correct. I'm fighting with getting the client installed to test it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use a git url with pip https://pip.pypa.io/en/latest/reference/pip_install.html#git to get the latest master of docker-py.
As a side note, I wonder if the config option should be dockerfile, to keep it consistent with the api key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within docker-py there's a utils.compare_version
def compare_version(v1, v2):
"""Compare docker versions
>>> v1 = '1.9'
>>> v2 = '1.10'
>>> compare_version(v1, v2)
1
>>> compare_version(v2, v1)
-1
>>> compare_version(v2, v2)
0
"""
s1 = StrictVersion(v1)
s2 = StrictVersion(v2)
if s1 == s2:
return 0
elif s1 > s2:
return -1
else:
return 1Do you know if there's a helper like this already within compose so I could add something like :
if 'dockerfile' in options and utils.compare_version('1.17', client.api_version()) is -1:
raise ConfigError('Docker client API must be greater than 1.17 to support the dockerfile option')and when building:
def build(self, no_cache=False):
log.info('Building %s...' % self.name)
build_kwars = dict(
tag=self.full_name,
stream=True,
rm=True,
nocache=no_cache,
)
if 'dockerfile' in self.options and \
utils.compare_version('1.17', self.client.api_version()) is not -1:
build_kwars['dockerfile'] = self.options.get('dockerfile', None)
build_output = self.client.build(
self.options['build'],
**build_kwars
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't, but I wouldn't worry about it. We're planning on requiring the latest docker to pickup some new features in 1.6 anyway.
|
I haven't tested this with unreleased docker-py, but hopefully it should work. The Let me know what you think, thanks for helping with this. |
|
Note: Not sure where the api version is coming from I did the following to update
|
|
Just saw your command about the version check (got squished under the change), let me know if you'd like me to remove the checks that I added. @dnephin |
|
Ya, I would remove it. If someone is an on earlier version, they can just not specify the option and it should continue to work without the dockerfile support. |
|
I tried to use this locally, but ran into some issues. I got the error about the API version, so I removed those checks, but now I'm getting $ docker version
Client version: 1.5.0
Client API version: 1.17
Go version (client): go1.4.1
Git commit (client): a8a31ef
OS/Arch (client): darwin/amd64
Server version: 1.5.0
Server API version: 1.17
Go version (server): go1.4.1
Git commit (server): a8a31efAnd I also updated docker-py to the latest commit. Is there anything else I need to do to get this working? Thanks! |
|
@mwean You need to install the version from github From: To: otherwise you'll get a version conflict. |
|
@dnephin I removed the checks, but would you like me to add the **build_kwargs portion back in. That way if they are on an outdated version of |
|
Maybe this? |
|
I think it's a lot less intuitive if a field can be a string or a mapping. It also makes it a lot harder (maybe impossible) to encode the expected structure of the configuration (in something like http://json-schema.org/ for example). If we were able to break backwards compatibility, and always do and build was never just a string, I think it would be nice. |
|
BTW, there is now a docker-py 1.1.0, so if you update the setup.py:install_requires and the requirements.txt , the tests should pass on docker 1.5+ |
|
Oh awesome, I'll give it a try today!!! |
|
It's not working for me, the container builds, but then exits on up will look into this more when I have time later, unless someone has an idea, thanks. |
|
This PR has been working well for me although I had to:
|
|
@loic you mean this line? compose/compose/cli/docker_client.py Line 35 in 4869fed
Sorry for the close PR phone miss clicked.. Not sure what was going on, I had to remove my docker images and it seems fine now after rebuilding. |
|
Needs rebasing now that #1100 is merged. |
|
Rebased with upstream/master |
|
@KyleJamesWalker yes that was the line. Maybe consider squashing into a single commit? |
|
@loic Didn't know about squashing, hope that did it correctly. Thanks for the tip! |
|
Looks like all the errors are:
@aanand do I need to do anything else to get this merged in? |
|
If you rebase on master, then remove all the Docker versions apart from 1.6.0-rc2, it should fix the build. |
|
@aanand Rebased. Also amended commit to include a properly formatted DCO marker. |
|
Dockerfile still needs updating to remove Docker versions 1.3.3, 1.4.1 and 1.5.0. |
|
+1 I can't wait for this |
|
+1 this is gonna be awesome |
|
+1 cool feature - we are waiting for it |
|
requests 2.6.1 is out, so try updating |
|
Thanks, I'll give it a try when I make it into work today! |
Signed-off-by: Kyle James Walker <KyleJamesWalker@gmail.com>
|
WOOOO!!!!!!!!!!! |
|
👍 |
|
LGTM |
|
LGTM! |
Support alternate Dockerfile name.
|
👍 |
|
👍👍👍👍👍👍👍👍👍👍👍 |
|
Nevermind, after digging more I see that this will be part of Docker Compose v1.3.0 which is slated to be released on June 26, 2015. |
|
Oh you beat me to it, can't wait for it in the main release! :P |
|
Docker Compose v1.3.0rc2 Homebrew Formula: https://gist.github.com/joost/20934650f68dd561fc35 |
|
Please note that Homebrew isn't an official installation method! Head here to install from binaries or pip: https://github.com/docker/compose/releases/tag/1.3.0rc2 |




Add support for compose support Docker's
-f/--fileoption from within your docker-compose.yml file.I quickly threw this in, and it seems to work for me, please let me know if anything else is needed to get this into master.