Skip to content

Add netmode to tests #691

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+28 −10
Closed

Add netmode to tests #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docker/utils/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,16 @@ def create_container_config(
if volumes_from is not None:
raise errors.InvalidVersion(message.format('volumes_from'))

# NetworkMode must be present and valid in host config from 1.20 onwards
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like that snippet of code belongs to create_host_config instead. What's the rationale for having it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you may well be right. I'd need to look at the flow more closely, but I believe there are cases where create_container_config is called without create_host_config. I'm not sure calling create_host_config from create_container_config is the right thing to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the user can decide to provide their own dictionary for host_config instead of using create_host_config, but at that point they are making the conscious choice of not perusing the library's validation and transformation features the library provides, which I believe docker-py should let them do without tampering. AFAIK the tests always use create_host_config.

The only drawback is that create_host_config doesn't have access to the client's API version. Not sure we have a way to work around that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure what I should do here in that case :)

if compare_version('1.20', version) >= 0:
if host_config is None:
host_config = {'NetworkMode': 'default'}
else:
if 'NetworkMode' not in host_config:
host_config['NetworkMode'] = 'default'
elif host_config['NetworkMode'] == '':
host_config['NetworkMode'] = 'default'

return {
'Hostname': hostname,
'Domainname': domainname,
Expand Down
28 changes: 18 additions & 10 deletions tests/integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ def runTest(self):
container = self.client.create_container(
'busybox',
['ls', mount_dest], volumes={mount_dest: {}},
host_config=create_host_config(binds=binds)
host_config=create_host_config(
binds=binds, network_mode='none')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we setting this explicitly here if it's already handled in create_container_config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you're right. Removing.

Edit: Looks like it needed here. Reason being because of the nesting of create_container and create_host_config. The test will fail without it being present.

)
container_id = container['Id']
self.client.start(container_id)
Expand Down Expand Up @@ -221,7 +222,8 @@ def runTest(self):
container = self.client.create_container(
'busybox',
['ls', mount_dest], volumes={mount_dest: {}},
host_config=create_host_config(binds=binds)
host_config=create_host_config(
binds=binds, network_mode='none')
)
container_id = container['Id']
self.client.start(container_id)
Expand Down Expand Up @@ -273,7 +275,8 @@ class TestCreateContainerReadOnlyFs(BaseTestCase):
def runTest(self):
ctnr = self.client.create_container(
'busybox', ['mkdir', '/shrine'],
host_config=create_host_config(read_only=True)
host_config=create_host_config(
read_only=True, network_mode='none')
)
self.assertIn('Id', ctnr)
self.tmp_containers.append(ctnr['Id'])
Expand Down Expand Up @@ -347,7 +350,8 @@ def runTest(self):
class TestCreateContainerPrivileged(BaseTestCase):
def runTest(self):
res = self.client.create_container(
'busybox', 'true', host_config=create_host_config(privileged=True)
'busybox', 'true', host_config=create_host_config(
privileged=True, network_mode='none')
)
self.assertIn('Id', res)
self.tmp_containers.append(res['Id'])
Expand Down Expand Up @@ -591,7 +595,8 @@ def runTest(self):

container = self.client.create_container(
'busybox', ['sleep', '60'], ports=list(port_bindings.keys()),
host_config=create_host_config(port_bindings=port_bindings)
host_config=create_host_config(
port_bindings=port_bindings, network_mode='bridge')
)
id = container['Id']

Expand Down Expand Up @@ -717,7 +722,8 @@ def runTest(self):
)
res2 = self.client.create_container(
'busybox', 'cat', detach=True, stdin_open=True,
host_config=create_host_config(volumes_from=vol_names)
host_config=create_host_config(
volumes_from=vol_names, network_mode='none')
)
container3_id = res2['Id']
self.tmp_containers.append(container3_id)
Expand Down Expand Up @@ -760,7 +766,8 @@ def runTest(self):

res2 = self.client.create_container(
'busybox', 'env', host_config=create_host_config(
links={link_path1: link_alias1, link_path2: link_alias2}
links={link_path1: link_alias1, link_path2: link_alias2},
network_mode='none'
)
)
container3_id = res2['Id']
Expand All @@ -781,7 +788,8 @@ class TestRestartingContainer(BaseTestCase):
def runTest(self):
container = self.client.create_container(
'busybox', ['sleep', '2'], host_config=create_host_config(
restart_policy={"Name": "always", "MaximumRetryCount": 0}
restart_policy={"Name": "always", "MaximumRetryCount": 0},
network_mode='none'
)
)
id = container['Id']
Expand Down Expand Up @@ -910,7 +918,7 @@ class TestCreateContainerWithHostPidMode(BaseTestCase):
def runTest(self):
ctnr = self.client.create_container(
'busybox', 'true', host_config=create_host_config(
pid_mode='host'
pid_mode='host', network_mode='none'
)
)
self.assertIn('Id', ctnr)
Expand Down Expand Up @@ -945,7 +953,7 @@ def runTest(self):

container2 = self.client.create_container(
'busybox', 'cat', host_config=create_host_config(
links={link_path: link_alias}
links={link_path: link_alias}, network_mode='none'
)
)
container2_id = container2['Id']
Expand Down