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

Fix moby/moby #37229 allowing to create networks even there are no services attached to it when deploying stacks #1120

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jaswdr
Copy link

@jaswdr jaswdr commented Jun 8, 2018

- What I did
Make docker stack deploy create networks even if there are no services attached to it, so you can create stack files for networks only
- How I did it
Change cli/compose/convert/compose.go to consider the networks map when servicesNetworks lenght is equals 0
- How to verify it
The issue #37229 in moby repo has more information about how to reproduce the error
- Description for the changelog
Support deploy network only stacks

pug

network := networks[networkName]
if network.External.External {
externalNetworks = append(externalNetworks, network.Name)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

if network.External.External {
    externalNetworks = append(externalNetworks, network.Name)
} else{
    result[networkName] = networkCreateOptions(namespace, network)
}

@silvin-lubecki
Copy link
Contributor

Thank you @jaswdr for this PR! The design is ok for me, what do you think of this feature @thaJeztah @vdemeester ?

@jaswdr jaswdr changed the title Fix #37229 Create networks even if there are no services attached to it when deploy stacks Fix moby/moby #37229 Allow to create networks even there are no services attached to it when deploying stacks Jun 11, 2018
@jaswdr jaswdr changed the title Fix moby/moby #37229 Allow to create networks even there are no services attached to it when deploying stacks Fix moby/moby #37229 allowing to create networks even there are no services attached to it when deploying stacks Jun 11, 2018
@thaJeztah
Copy link
Member

Design works for me as well; I don't think we stated that a compose file must contain services, so a file that only has networks (or other objects) should work.

I see the second commit doesn't have a signed-off-by, so DCO check is failing @jaswdr could you squash your commits to make the DCO check pass?

@thaJeztah
Copy link
Member

We should also have a test added for this 👍

@jaswdr
Copy link
Author

jaswdr commented Jun 12, 2018

of course @thaJeztah, sorry for this, I will correct this now

Signed-off-by: Jonathan A. Schweder <[email protected]>
@jaswdr
Copy link
Author

jaswdr commented Jun 12, 2018

@thaJeztah done

@jaswdr
Copy link
Author

jaswdr commented Jun 12, 2018

test created

namespace := Namespace{name: "foo"}
serviceNetworks := map[string]struct{}{}
source := networkMap{
"normal": composetypes.NetworkConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

I think only one NetworkConfig, whatever it is, is enough to test the feature? The test would be then more compact and less redundant.

Copy link
Author

@jaswdr jaswdr Jun 13, 2018

Choose a reason for hiding this comment

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

@silvin-lubecki did you mean, add the content of the TestNetworksCreatedWithoutServices into the TestNetworks?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I meant remove all network configs except one, like named, in this test 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this should do the job I guess

func TestNetworksCreatedWithoutServices(t *testing.T) {
	namespace := Namespace{name: "foo"}
	serviceNetworks := map[string]struct{}{}
	source := networkMap{
		"named": composetypes.NetworkConfig{
			Name: "othername",
		},
	}
	expected := map[string]types.NetworkCreate{
		"named": {
			Labels: map[string]string{
				LabelNamespace: "foo",
			},
		},
	}

	networks, externals := Networks(namespace, source, serviceNetworks)
	assert.DeepEqual(t, expected, networks)
	assert.DeepEqual(t, []string{"special"}, externals)
}

WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Agree, I'll change this and remove the unecessary content

@jaswdr
Copy link
Author

jaswdr commented Jun 13, 2018

@silvin-lubecki simplified test 😄

@silvin-lubecki
Copy link
Contributor

Thank you @jaswdr 👍 !
LGTM

@vdemeester
Copy link
Collaborator

The problem I can think of with this PR is that it doesn't act the same as docker-compose.
Using a composefile without services (but network) with docker-compose has almost the same behavior as the current docker stack deploy : it won't create any network.

That said I would be in favor of this PR so let me ping @shin- @dnephin and @thaJeztah to have their thoughts on that 👼

@jaswdr
Copy link
Author

jaswdr commented Jun 13, 2018

@vdemeester if I make another PR to compose to handle the same behavior has this one, did you agree it is ok?

@vdemeester
Copy link
Collaborator

@jaswdr yes 👼 I wanted to have @shin- @thaJeztah or @dnephin's opinion before suggesting you that though (so that you don't work on something if we don't reach consensus on whether we wanna do that or not).

@thaJeztah
Copy link
Member

Yes we should match the behavior in compose as well (also see my earlier comment #1120 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants