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 docker's client's --add-host command #848

Closed
wants to merge 6 commits into from
Closed

Add support for docker's client's --add-host command #848

wants to merge 6 commits into from

Conversation

sampwing
Copy link

Updated docker-py to the most recent version ( 0.7.1), which allows assigning values to extra_hosts (similar to ---add-host.) Updated fig to allow passing in these parameters and added tests.

Implementation requested in the following issues:
#597
#754

Signed-off-by: Sam Wing <sampwing@gmail.com>
…he --add-host flag from the docker client

Signed-off-by: Sam Wing <sampwing@gmail.com>
@sampwing sampwing closed this Jan 15, 2015
@sampwing sampwing reopened this Jan 15, 2015
@sampwing
Copy link
Author

Kinda stumped on this I test locally (script/test) and I don't get the two failures wercker ran into. Currently running:
Docker version 1.4.1, build 5bc2ff8

@dnephin
Copy link

dnephin commented Jan 19, 2015

@sampwing wercker runs Docker 1.2.1 I believe https://github.com/wercker/wercker-labs-docker

I'm not sure if we can convince them to upgrade to latest, or if we'd have to create a custom box to run the tests.

@sampwing
Copy link
Author

@dnephin I could look into creating a box if that could push this along - 1.2 is a bit out of date, fig seems to be recommending 1.4 on irc

@aanand
Copy link

aanand commented Jan 21, 2015

I had a quick go yesterday at creating a custom Wercker box with Docker installed, but it failed with an error:

Docker found, this is not supported in the default wercker environment. See for more information: http://devcenter.wercker.com/articles/docker/

This suggests that you're not allowed to build a custom box with Docker in it. I've asked them about it on Twitter - we'll see.

@thaJeztah
Copy link
Member

@aanand given that fig/compose is part of docker, don't you have access to the build servers a Docker itself is using?

Fix when pyyaml has interpreted line as a dictionary
@tdesvenain
Copy link

Hi, what is the review status of this pull request ? What is the next step ? I can help.
Thanks

@dnephin
Copy link

dnephin commented Feb 18, 2015

It needs a rebase and squash, but it's also blocked on #886

@ch3lo
Copy link

ch3lo commented Feb 24, 2015

+1

extra_hosts_dict.update(extra_hosts_line)
else:
# not already interpreted as a dict
extra_hosts_dict.update(extra_hosts_line.split(':'))

Choose a reason for hiding this comment

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

I believe line 651 should read:

extra_hosts_dict.update([extra_hosts_line.split(':')])

Choose a reason for hiding this comment

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

Actually this introduces a bug

I just commited a fix + some unit tests, could you review them please ?
https://github.com/tdesvenain/fig/commit/3e6c7cd5a49a52f68184a3d784066d00ab377c1d

2015-02-25 13:27 GMT+01:00 ddunwoody notifications@github.com:

In fig/service.py
#848 (comment):

@@ -629,6 +634,25 @@ def split_port(port):
return internal_port, (external_ip, external_port or None)

+def build_extra_hosts(extra_hosts_config):

  • if extra_hosts_config is None:
  •    return None
    
  • if isinstance(extra_hosts_config, basestring):
  •    extra_hosts_config = [extra_hosts_config]
    
  • extra_hosts_dict = {}
  • for extra_hosts_line in extra_hosts_config:
  •    if isinstance(extra_hosts_line, dict):
    
  •        # already interpreted as a dict (depends on pyyaml version)
    
  •        extra_hosts_dict.update(extra_hosts_line)
    
  •    else:
    
  •        # not already interpreted as a dict
    
  •        extra_hosts_dict.update(extra_hosts_line.split(':'))
    

I believe line 651 should read:

extra_hosts_dict.update([extra_hosts_line.split(':')])

Reply to this email directly or view it on GitHub
https://github.com/docker/fig/pull/848/files#r25335828.

Thomas Desvenain

Téléphone : 09 51 37 35 18

@skrassiev
Copy link

hope this is the right way for +1

@albers
Copy link

albers commented Mar 14, 2015

+1

1 similar comment
@phylake
Copy link

phylake commented Mar 21, 2015

+1

@c0deaddict
Copy link

+1

1 similar comment
@bmilesp
Copy link

bmilesp commented Apr 3, 2015

+1

@aanand
Copy link

aanand commented Apr 7, 2015

Needs a rebase. docker-py changes can probably go.

@slashmili
Copy link

+1

2 similar comments
@jelmstrom
Copy link

+1

@tristan0x
Copy link

+1

@aanand
Copy link

aanand commented Apr 30, 2015

Superseded by #1158

@aanand aanand closed this Apr 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet