Make compose file allow to specify names for non-external volume by lipingxue · Pull Request #306 · docker/cli

lipingxue · 2017-07-06T18:09:51Z

- What I did
Fixes #274

- How I did it
I changed the config_schema.json to allow names for non-external volumes, and also made change in function convertVolumeToMount accordingly.
- How to verify it

Try docker stack deploy with the following yaml file(specify customized volume name for non-external volume), and it works as expected

root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# cat /root/postgres2.yml 
version: "3.3"
 
services:
 
  postgres:
    image: postgres
    ports:
      - "5432:5432"
    volumes:
      - postgres_vol:/var/lib/data
    environment:
      - "POSTGRES_PASSWORD=secretpass"
      - "PGDATA=/var/lib/data/db"
    deploy:
      restart_policy:
        condition: on-failure
      placement:
        constraints:
          - node.role == worker
volumes:
   postgres_vol:
      name: "postgres_vol@sharedVmfs-1"
      driver: "vsphere"
      driver_opts:
        size: "1GB"
root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# 


root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# ./docker-linux-amd64 stack deploy -c /root/postgres2.yml postgres
Creating network postgres_default
Creating service postgres_postgres
root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# docker volume ls
DRIVER              VOLUME NAME
vsphere:latest      postgres_vol@sharedVmfs-1

root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# docker service ps postgres_postgres 
ID            NAME                 IMAGE            NODE                       DESIRED STATE  CURRENT STATE           ERROR  PORTS
qx2nvqqwcqyu  postgres_postgres.1  postgres:latest  sc-rdops-vm02-dhcp-52-237  Running        Running 12 seconds ago

Try docker stack deploy with the following yaml file(specify volume name for external volume), and it returns with proper error

root@sc-rdops-vm02-dhcp-52-237:~# cat postgres_external.yml 
version: "3.3"
 
services:
 
  postgres:
    image: postgres
    ports:
      - "5432:5432"
    volumes:
      - postgres_vol:/var/lib/data
    environment:
      - "POSTGRES_PASSWORD=secretpass"
      - "PGDATA=/var/lib/data/db"
    deploy:
      restart_policy:
        condition: on-failure
      placement:
        constraints:
          - node.role == worker
volumes:
   postgres_vol:
     external:
       name: "postgres@sharedVmfs-1"


root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# ./docker-linux-amd64 stack deploy -c /root/postgres_external.yml postgres
Creating network postgres_default
service postgres: cannot specify external volume name "postgres@sharedVmfs-1"

root@sc-rdops-vm02-dhcp-52-237:~/tmp_with_fix# docker stack ls
NAME  SERVICES

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

dnephin

Thanks!

This looks like a good start. This change will need to be moved to a v3.4 version of the schema, but that doesn't exist yet, so I can take care of it later.

I think we should make this change to networks/configs/secrets as well, so they are all consistent. I can take care of that as well in another PR.

dnephin · 2017-07-07T14:35:21Z

cli/compose/convert/volume.go

-	// External named volumes
-	if stackVolume.External.External {
-		result.Source = stackVolume.External.Name
-		return result, nil


We still need this block to support External.External.

We could also add a deprecation message in the loader

@dnephin Thanks for your review.

I have added the block back. Could you tell me how to add a deprecation message in the loader? I am not familiar with the loader code.

ci/circleci: validate test failed. It fails in some vendor Makefile, but I haven't changed anything related to this. Could you help me to take a look? Thanks!

I think the warning would go around here:

cli/cli/compose/loader/loader.go

Line 447 in 732261f

}

Probably as an else to that if block

I think the validate problem was fixed on master, so if you rebase it should be fixed.

codecov-io · 2017-07-07T17:52:09Z

Codecov Report

Merging #306 into master will decrease coverage by 0.64%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
- Coverage   46.14%   45.49%   -0.65%     
==========================================
  Files         193      193              
  Lines       16073    16069       -4     
==========================================
- Hits         7417     7311     -106     
- Misses       8269     8380     +111     
+ Partials      387      378       -9

lipingxue · 2017-07-07T18:06:56Z

@dnephin I have addressed your comments and also have some questions for you. Please take a look. Thanks!

dnephin · 2017-07-12T18:28:09Z

cli/compose/loader/loader.go

 				volume.External.Name = name
 				volumes[name] = volume
+			} else {
+				return nil, externalVolumeError(name, "name")


Sorry, I think this should be just a warning, not an error.

You can print a warning with logrus.Warnf()

lipingxue · 2017-07-14T17:08:47Z

@dnephin I have addressed your comments, and please take a look. Thanks.

dnephin

Thanks! I've pushed a commit to add a schema for 3.4 and rebased your commit on top of it.

I also adjusted the wording on the warning slightly, and made it an error to set both names.

I think now we just need to add to the full test in loader_test.go and a test in convert/volume_test.go.

lipingxue · 2017-07-17T16:28:42Z

@dnephin Thanks. I will look at loader_test.go and convert/volume_test.go to figure out how to add the test.

lipingxue · 2017-07-18T20:22:14Z

@dnephin I have added unit test and repushed the commit. Please review it. Thanks!

Signed-off-by: Daniel Nephin <dnephin@docker.com>

lipingxue · 2017-07-19T21:28:16Z

@dnephin I have rebased to get v3.4 schema and addressed your comments. Please review it. Thanks!

thaJeztah

Thanks @lipingxue - this is great! I left some comments

@dnephin for consistency; should the same change be applied to networks, configs, and secrets? (i.e., network.name instead of network.external.name)?

thaJeztah · 2017-07-20T10:50:21Z

cli/compose/loader/full-example.yml

+
  external-volume:
    # Specifies that a pre-existing volume called "external-volume"
    # can be referred to within this file as "external-volume"


for other-external-volume below, should we update the comment to mention volume.external.name is deprecated? (can't comment on that line, so leaving the comment here 😄)

# This example uses the deprecated "volume.external.name" (replaced by "volume.name")

Done. Add the comment.

thaJeztah · 2017-07-20T10:51:24Z

cli/compose/loader/full-example.yml

+      # Values can be strings or numbers
+      foo: "bar"
+      baz: 1
+


I'd like to see an example using a name and external, for example;

external-volume3: name: this-is-volume3 external: true

Also (not sure if that needs to be done in this file, or another one); can we have a test that tests if setting both volume.name and volume.external.name produces the "conflicting options" error? i.e.;

external-volume4-fail: # name cannot be provided both as volume.name and volume.external.name name: custom-name external: name: conflicting-name

A separate test case for testing the conflict would be great

Done. Add a test case in for that.

thaJeztah · 2017-07-20T10:55:07Z

cli/compose/types/types.go


 // VolumeConfig for a volume
 type VolumeConfig struct {
+	Name       string


@dnephin should External.Name below (https://github.com/docker/cli/pull/306/files#diff-80e4727ffc215ce8081027b2780b1494L317) be marked deprecated ?

That link isn't going to any specific line. I don't think we should add it to the lists in types.go, those are for options being deprecated/removed from v2->v3.

In this case maybe just a comment in the struct would be fine

Updating the docs, and mentioning that could be ok?

Done. Add a comment in the struct.

dnephin · 2017-07-21T15:08:40Z

for consistency; should the same change be applied to networks, configs, and secrets?

Yes, but I was going to handle it after this PR merges. I think we need to refactor these objects to share more code.

dnephin · 2017-07-21T15:11:43Z

hmm, webhooks didn't trigger on this PR either. This is becoming a problem

lipingxue · 2017-07-21T18:52:18Z

@thaJeztah I have addressed your comments, and please review it.

mdlinville · 2017-07-24T20:52:31Z

cli/compose/loader/loader.go

 				volume.External.Name = name
 				volumes[name] = volume
+			} else {
+				logrus.Warnf("volume %s: volume.external.name is deprecated, please use volume.name", name)


Maybe "is deprecated in favor of", to avoid using "please" and also avoid the comma splice.

mdlinville · 2017-07-24T20:52:59Z

cli/compose/loader/loader.go

+				logrus.Warnf("volume %s: volume.external.name is deprecated, please use volume.name", name)
+
+				if volume.Name != "" {
+					return nil, errors.Errorf("volume %s: volume.external.name and volume.name conflict, only use volume.name", name)


This also has a comma splice. It should be two sentences, or a semicolon instead of the comma.

mdlinville · 2017-07-24T20:53:14Z

cli/compose/loader/loader_test.go

+
+	assert.Error(t, err)
+	fmt.Println(err)
+	assert.Contains(t, err.Error(), "volume.external.name and volume.name conflict, only use volume.name")


Comma splice.

lipingxue · 2017-07-26T16:34:24Z

@mstanleyjones I have addressed your comments, and please take a look.

dnephin

LGTM

thaJeztah

LGTM, but can you squash your commits ? Thinking of one commit for the compose-file 3.4 bump, and one commit for the other changes

Signed-off-by: Liping Xue <lipingxue@gmail.com> Change to enable volume name can be customized. Signed-off-by: Liping Xue <lipingxue@gmail.com> Change to enable volume name can be customized. Remove unused debug info. Address comments from Daniel and solve the lint error. Signed-off-by: Liping Xue <lipingxue@gmail.com> Address Daniel's comments to print warning message when name of external volume is set in loader code. Signed-off-by: Liping Xue <lipingxue@gmail.com> Address Daniel's comments to return error when external volume is set in loader code. Signed-off-by: Liping Xue <lipingxue@gmail.com> Address Daniel's comments to return error when external volume is set in loader code. Signed-off-by: Liping Xue <lipingxue@gmail.com> Remove the case that specifying external volume name in full-example.yml. More fix. Add unit test. Signed-off-by: Liping Xue <lipingxue@gmail.com> Address comments from Daniel, move the schema change to v3.4. Signed-off-by: Liping Xue <lipingxue@gmail.com> Address comments from Sebastiaan. Signed-off-by: Liping Xue <lipingxue@gmail.com> Address comments from Misty. Signed-off-by: Liping Xue <lipingxue@gmail.com>

lipingxue · 2017-07-27T22:13:21Z

@thaJeztah I have squashed the comments, please review it. Thanks!

thaJeztah

LGTM, thanks!

This change needs an update to the documentation; probably in the vnext-compose branch; https://github.com/docker/docker.github.io/tree/vnext-compose/compose/compose-file

but ping @londoncalling for confirmation

also ping @shin- for the implementation in docker-compose

hairyhenderson · 2017-07-27T23:08:53Z

This is awesome! Thanks @lipingxue 🎉

kvncrw · 2017-09-28T20:21:31Z

@dnephin is there an issue open regarding?

networks, configs, and secrets

dnephin · 2017-09-28T20:34:50Z

I don't think there are any open issues for the other objects

GordonTheTurtle added dco/no status/0-triage labels Jul 6, 2017

lipingxue changed the title ~~New compose fix.liping~~ Make compose file allow to specify names for non-external volume Jul 6, 2017

lipingxue force-pushed the new_compose_fix.liping branch from d671412 to 57a44fc Compare July 6, 2017 21:10

GordonTheTurtle removed the dco/no label Jul 6, 2017

dnephin reviewed Jul 7, 2017

View reviewed changes

GordonTheTurtle added the dco/no label Jul 7, 2017

lipingxue force-pushed the new_compose_fix.liping branch from ee57690 to 00f39c9 Compare July 7, 2017 17:52

GordonTheTurtle removed the dco/no label Jul 7, 2017

lipingxue mentioned this pull request Jul 7, 2017

compose yml with datastore selection vmware-archive/vsphere-storage-for-docker#1315

Closed

shouze mentioned this pull request Jul 11, 2017

[Compose file] allow names for non-external volumes #274

Closed

lipingxue force-pushed the new_compose_fix.liping branch from 00f39c9 to 04c81ed Compare July 11, 2017 23:25

dnephin reviewed Jul 12, 2017

View reviewed changes

lipingxue force-pushed the new_compose_fix.liping branch 2 times, most recently from 5d0dc47 to b6fc974 Compare July 12, 2017 19:04

dnephin added status/2-code-review and removed status/0-triage labels Jul 14, 2017

dnephin force-pushed the new_compose_fix.liping branch 2 times, most recently from ee34a30 to 9ada891 Compare July 14, 2017 21:20

dnephin reviewed Jul 14, 2017

View reviewed changes

docker deleted a comment from GordonTheTurtle Jul 17, 2017

vdemeester self-requested a review July 17, 2017 07:38

dgageot mentioned this pull request Jul 17, 2017

Add build.network field to Compose file v3.x #305

Closed

dnephin mentioned this pull request Jul 18, 2017

Include an "order" key in the "update_config" #346

Closed

lipingxue force-pushed the new_compose_fix.liping branch from 9ada891 to e7c296a Compare July 18, 2017 18:27

Add Compose format 3.4

0adccac

Signed-off-by: Daniel Nephin <dnephin@docker.com>

lipingxue force-pushed the new_compose_fix.liping branch from 7a63629 to ab4de41 Compare July 19, 2017 21:26

thaJeztah requested changes Jul 20, 2017

View reviewed changes

thaJeztah added impact/changelog impact/deprecation impact/documentation labels Jul 20, 2017

mdlinville suggested changes Jul 24, 2017

View reviewed changes

mdlinville approved these changes Jul 26, 2017

View reviewed changes

dnephin approved these changes Jul 26, 2017

View reviewed changes

thaJeztah requested changes Jul 27, 2017

View reviewed changes

thaJeztah added the impact/compose-file label Jul 27, 2017

lipingxue force-pushed the new_compose_fix.liping branch from 7761b1b to 27a3080 Compare July 27, 2017 22:12

thaJeztah approved these changes Jul 27, 2017

View reviewed changes

thaJeztah added the docs/revisit label Jul 27, 2017

thaJeztah added this to the 17.08.0 milestone Jul 27, 2017

thaJeztah merged commit 7524912 into docker:master Jul 27, 2017

shin- mentioned this pull request Jul 27, 2017

[v3.4] Volume name field docker/compose#5059

Closed

dnephin mentioned this pull request Jul 28, 2017

[WIP] Add Compose format 3.4 #358

Closed

lipingxue mentioned this pull request Jul 31, 2017

Add test to verify docker compose file can specify volume with format "vol@datastore". vmware-archive/vsphere-storage-for-docker#1666

Closed

thaJeztah mentioned this pull request Mar 3, 2018

Secret names should be able to include variables moby/moby#35229

Closed

Comments

Conversation

lipingxue commented Jul 6, 2017

Uh oh!

dnephin 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!

codecov-io commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lipingxue commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lipingxue commented Jul 14, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

lipingxue commented Jul 17, 2017

Uh oh!

lipingxue commented Jul 18, 2017

Uh oh!

lipingxue commented Jul 19, 2017

Uh oh!

thaJeztah 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!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jul 21, 2017

Uh oh!

dnephin commented Jul 21, 2017

Uh oh!

lipingxue commented Jul 21, 2017

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!

lipingxue commented Jul 26, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

lipingxue commented Jul 27, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 7, 2017 •

edited

Loading