Add support for creating volume and network with label definition by metayd · Pull Request #3898 · docker/compose

metayd · 2016-09-01T12:44:57Z

- What I did
Add volume and network label support for compose file v2

- How I did it

Add labels to volume and network section in config_schema_v2.0.json
Add implementation
Add doc of volume and network label to docs/compose-file.md

-How to verify it
Add test in tests/unit/config/config_test.py, tests/integration/project_test.py and tests/acceptance/cli_test.py

@shin- PTAL

Signed-off-by: dbdd wangtong2712@gmail.com

shin- · 2016-09-01T23:27:10Z

tests/integration/project_test.py

+            v for v in self.client.volumes().get('Volumes', [])
+            if v['Name'].startswith('composetest_')
+        ]
+


nit: we don't need sorted() here since there's a single element

shin- · 2016-09-01T23:39:54Z

Thanks! This is quite thourough. There's a few things I see:

I have one minor nitpick on the test code, see above.
We need to update the docker-py dependency in requirements.txt. We can use 235964607d200b57ea41383e54473c2976f4d730
Since this requires API 1.23, we may actually want to / have to make it part of a 2.1 compose file spec. It's actually something we've already done for another option in Add support for link-local IPs in service.networks definition #3653 , but just hasn't been merged yet. cc @dnephin @aanand on this.

metayd · 2016-09-02T00:30:40Z

PR updated. PTAL @shin-

Fixes nits
Update docker-py dependency to 235964607d200b57ea41383e54473c2976f4d730

Do I need to use @v2_1_only in the releated test code and add [Version 2.1 file format] to the releated doc? or do these things after when #3653 is merged?

alexmavr · 2016-09-06T18:28:12Z

@dbdd4us are you planning to address the network part of #3892 as well? This PR only seems to address volume labels

metayd · 2016-09-09T10:31:35Z

@alexmavr
I will add the network part as soon as possible. : )

I'm sorry I close this pr by accident just now.

metayd · 2016-09-17T11:48:51Z

PR updated. PTAL @alexmavr @shin-

Add network part
Update docker-py dependency to 1.10.2

I was wondering if I need to use @v2_1_only in the releated test code and add [Version 2.1 file format] to the releated doc? or do these things after when #3653 is merged?

shin-

Hey @dbdd4us - sorry it took so long to get back to you. I wanted to have the changes from #3653 merged before we move forward with this.

I added a few comments to highlight the changes that need to be made to make the labels directives part of the 2.1 file format. They're trivial changes, but let me know if anything is unclear and I'll be happy to help. :)

shin- · 2016-09-23T23:08:06Z

compose/config/config_schema_v2.0.json

        },
-        "internal": {"type": "boolean"}
+        "internal": {"type": "boolean"},
+        "labels": {"$ref": "#/definitions/list_or_dict"}


Now that config_schema_v2.1.json has been merged, we can move this property there.

shin- · 2016-09-23T23:08:14Z

compose/config/config_schema_v2.0.json

            "name": {"type": "string"}
          }
        },
+        "labels": {"$ref": "#/definitions/list_or_dict"},


Now that config_schema_v2.1.json has been merged, we can move this property there.

shin- · 2016-09-23T23:10:28Z

requirements.txt

 backports.ssl-match-hostname==3.5.0.1; python_version < '3'
 cached-property==1.2.0
-docker-py==1.9.0
+docker-py==1.10.2


You'll probably get a merge conflict here since we bumped to 1.10.3 in master.

shin- · 2016-09-23T23:11:22Z

tests/acceptance/cli_test.py

@@ -754,6 +754,46 @@ def test_up_with_external_default_network(self):
        container = self.project.containers()[0]
        assert list(container.get('NetworkSettings.Networks')) == [network_name]



Update this to @v2_1_only()

shin- · 2016-09-23T23:11:35Z

tests/acceptance/cli_test.py

+        assert [n['Name'] for n in networks] == [network_with_label]
+
+        assert networks[0]['Labels'] == {'label_key': 'label_val'}
+


Update this to @v2_1_only()

shin- · 2016-09-23T23:12:04Z

tests/integration/project_test.py

@@ -783,6 +783,42 @@ def test_project_up_with_network_internal(self):

        assert network['Internal'] is True



Update this to @v2_1_only()

shin- · 2016-09-23T23:12:12Z

tests/integration/project_test.py

@@ -809,6 +845,46 @@ def test_project_up_volumes(self):
        self.assertEqual(volume_data['Name'], full_vol_name)
        self.assertEqual(volume_data['Driver'], 'local')



Update this to @v2_1_only()

shin- · 2016-09-23T23:18:16Z

docs/compose-file.md

          name: actual-name-of-volume

+### labels
+


Needs the > Added in version 2.1 file format line, similar to this

shin- · 2016-09-23T23:18:29Z

docs/compose-file.md

          name: actual-name-of-network

+### labels
+


Needs the > Added in version 2.1 file format line, similar to this

metayd · 2016-09-26T11:35:21Z

@shin- PR Updated, PTAL

I'm confused about the Reorder python imports Failed in Jenkins build.

dnephin · 2016-09-26T15:21:33Z

"Reorder python imports" is a lint/style check performed by pre-commit. It means the imports are not ordered correctly. You can either install http://pre-commit.com/ and run it to fix the issue, or you can re-order them manually.

Imports should be sorted and grouped according to "built-in", "third-party", "local"

I think in this case it doesn't like the multiple imports on a single line, and is moving it onto a new line

metayd · 2016-09-27T03:54:19Z

@dnephin
Thanks, it has been fixed. PR Updated, PTAL : )

shin-

LGTM!

shin- · 2016-10-06T22:59:26Z

WIth the docs moving to a separate repository, this needs a rebase that removes the docs/compose-file.md. @dbdd4us , do you mind taking care of it? Thanks, and sorry about the delay on merging this.

metayd · 2016-10-07T03:22:11Z

@shin-
Rebased, but it seems that jenkins ci is broken, I'm not sure if it's accidental.

Signed-off-by: dbdd <wangtong2712@gmail.com>

allencloud · 2016-10-14T07:57:23Z

LGTM (IANAM)
Any Update?
Thanks

volkyeth · 2016-10-14T09:48:08Z

Closes #3107

aanand

LGTM

GordonTheTurtle added status/0-triage dco/no labels Sep 1, 2016

metayd force-pushed the master branch from c517eed to 5ec9ac9 Compare September 1, 2016 12:46

GordonTheTurtle removed the dco/no label Sep 1, 2016

metayd force-pushed the master branch from 5ec9ac9 to 218ba98 Compare September 1, 2016 12:56

shin- reviewed Sep 1, 2016
View reviewed changes

metayd force-pushed the master branch from 218ba98 to 03559b7 Compare September 2, 2016 00:20

metayd closed this Sep 9, 2016

metayd reopened this Sep 9, 2016

metayd changed the title ~~Add support for creating volume with label definition~~ Add support for creating volume and network with label definition Sep 17, 2016

metayd force-pushed the master branch from a95a7e1 to 63119e2 Compare September 17, 2016 11:44

shin- reviewed Sep 23, 2016

View reviewed changes

metayd force-pushed the master branch 3 times, most recently from 73da01e to 0fcdeb3 Compare September 26, 2016 11:19

metayd force-pushed the master branch from 0fcdeb3 to 7c7d7eb Compare September 27, 2016 02:59

shin- added this to the 1.9.0 milestone Sep 28, 2016

shin- approved these changes Sep 28, 2016

View reviewed changes

metayd force-pushed the master branch from 7c7d7eb to 76addaf Compare October 7, 2016 02:56

metayd force-pushed the master branch from 76addaf to 0045620 Compare October 12, 2016 02:33

GordonTheTurtle added the dco/no label Oct 12, 2016

add support for creating volume and network with label definition

bdcce13

Signed-off-by: dbdd <wangtong2712@gmail.com>

metayd force-pushed the master branch from 0045620 to bdcce13 Compare October 12, 2016 02:35

GordonTheTurtle added dco/no and removed dco/no labels Oct 12, 2016

metayd force-pushed the master branch from ca75237 to bdcce13 Compare October 12, 2016 03:06

GordonTheTurtle removed dco/no labels Oct 12, 2016

aanand approved these changes Oct 17, 2016

View reviewed changes

shin- merged commit bdcce13 into docker:master Oct 17, 2016

scic mentioned this pull request Oct 18, 2016

Proposal: Support for building images with labels #4038

Closed

		@@ -754,6 +754,46 @@ def test_up_with_external_default_network(self):
		container = self.project.containers()[0]
		assert list(container.get('NetworkSettings.Networks')) == [network_name]

		assert [n['Name'] for n in networks] == [network_with_label]

		assert networks[0]['Labels'] == {'label_key': 'label_val'}

		@@ -783,6 +783,42 @@ def test_project_up_with_network_internal(self):

		assert network['Internal'] is True

		@@ -809,6 +845,46 @@ def test_project_up_volumes(self):
		self.assertEqual(volume_data['Name'], full_vol_name)
		self.assertEqual(volume_data['Driver'], 'local')

Conversation

metayd commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shin- commented Sep 1, 2016

Uh oh!

metayd commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexmavr commented Sep 6, 2016

Uh oh!

metayd commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metayd commented Sep 17, 2016

Uh oh!

shin- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metayd commented Sep 26, 2016

Uh oh!

dnephin commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metayd commented Sep 27, 2016

Uh oh!

shin- left a comment

Choose a reason for hiding this comment

Uh oh!

shin- commented Oct 6, 2016

Uh oh!

metayd commented Oct 7, 2016

Uh oh!

allencloud commented Oct 14, 2016

Uh oh!

volkyeth commented Oct 14, 2016

Uh oh!

aanand left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

metayd commented Sep 1, 2016 •

edited

Loading

metayd commented Sep 2, 2016 •

edited

Loading

metayd commented Sep 9, 2016 •

edited

Loading

dnephin commented Sep 26, 2016 •

edited

Loading