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

Make compose file allow to specify names for non-external volume #306

Merged
merged 2 commits into from Jul 27, 2017

Conversation

@lipingxue
Copy link
Contributor

commented Jul 6, 2017

- 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

  1. 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        
  1. 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)
image

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

@lipingxue lipingxue force-pushed the lipingxue:new_compose_fix.liping branch from d671412 to 57a44fc Jul 6, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jul 6, 2017

@dnephin
Copy link
Collaborator

left a comment

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.

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

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 7, 2017

Collaborator

We still need this block to support External.External.

We could also add a deprecation message in the loader

This comment has been minimized.

Copy link
@lipingxue

lipingxue Jul 7, 2017

Author Contributor

@dnephin Thanks for your review.

  1. 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.
  2. 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!

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 11, 2017

Collaborator

I think the warning would go around here:

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.

@lipingxue lipingxue force-pushed the lipingxue:new_compose_fix.liping branch from ee57690 to 00f39c9 Jul 7, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jul 7, 2017

@codecov-io

This comment has been minimized.

Copy link

commented Jul 7, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2017

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

@lipingxue lipingxue force-pushed the lipingxue:new_compose_fix.liping branch from 00f39c9 to 04c81ed Jul 11, 2017

@@ -444,6 +444,8 @@ func LoadVolumes(source map[string]interface{}) (map[string]types.VolumeConfig,
if volume.External.Name == "" {
volume.External.Name = name
volumes[name] = volume
} else {
return nil, externalVolumeError(name, "name")

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 12, 2017

Collaborator

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

You can print a warning with logrus.Warnf()

@lipingxue lipingxue force-pushed the lipingxue:new_compose_fix.liping branch 2 times, most recently from 5d0dc47 to b6fc974 Jul 12, 2017

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2017

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

@dnephin dnephin force-pushed the lipingxue:new_compose_fix.liping branch 2 times, most recently from ee34a30 to 9ada891 Jul 14, 2017

@dnephin
Copy link
Collaborator

left a comment

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.

@docker docker deleted a comment from GordonTheTurtle Jul 17, 2017

@vdemeester vdemeester self-requested a review Jul 17, 2017

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

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

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

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

@lipingxue lipingxue force-pushed the lipingxue:new_compose_fix.liping branch from 7a63629 to ab4de41 Jul 19, 2017

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

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

@thaJeztah
Copy link
Member

left a comment

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)?

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

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 20, 2017

Member

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")

This comment has been minimized.

Copy link
@lipingxue

lipingxue Jul 21, 2017

Author Contributor

Done. Add the comment.

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 20, 2017

Member

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

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 20, 2017

Member

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

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 21, 2017

Collaborator

A separate test case for testing the conflict would be great

This comment has been minimized.

Copy link
@lipingxue

lipingxue Jul 21, 2017

Author Contributor

Done. Add a test case in for that.

@@ -305,6 +305,7 @@ type IPAMPool struct {

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

This comment has been minimized.

Copy link
@thaJeztah

This comment has been minimized.

Copy link
@dnephin

dnephin Jul 21, 2017

Collaborator

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

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 21, 2017

Member

Updating the docs, and mentioning that could be ok?

This comment has been minimized.

Copy link
@lipingxue

lipingxue Jul 24, 2017

Author Contributor

Done. Add a comment in the struct.

@dnephin

This comment has been minimized.

Copy link
Collaborator

commented Jul 21, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 21, 2017

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

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2017

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

@@ -444,6 +444,12 @@ func LoadVolumes(source map[string]interface{}) (map[string]types.VolumeConfig,
if volume.External.Name == "" {
volume.External.Name = name
volumes[name] = volume
} else {
logrus.Warnf("volume %s: volume.external.name is deprecated, please use volume.name", name)

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Jul 24, 2017

Contributor

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

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 comment has been minimized.

Copy link
@mistyhacks

mistyhacks Jul 24, 2017

Contributor

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


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

This comment has been minimized.

Copy link
@mistyhacks

mistyhacks Jul 24, 2017

Contributor

Comma splice.

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

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

@dnephin
Copy link
Collaborator

left a comment

LGTM

@thaJeztah
Copy link
Member

left a comment

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

Change to enable volume name can be customized.
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 lipingxue force-pushed the lipingxue:new_compose_fix.liping branch from 7761b1b to 27a3080 Jul 27, 2017

@lipingxue

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2017

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

@thaJeztah
Copy link
Member

left a comment

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

@thaJeztah thaJeztah added this to the 17.08.0 milestone Jul 27, 2017

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

8 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project Absolute coverage decreased by -0.64% but relative coverage increased by +53.85% compared to 2dac00b
Details
dco-signed All commits are signed
@shin- shin- referenced this pull request Jul 27, 2017
@hairyhenderson

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

This is awesome! Thanks @lipingxue 🎉

@notsureifkevin

This comment has been minimized.

Copy link

commented Sep 28, 2017

@dnephin is there an issue open regarding?

networks, configs, and secrets

@dnephin

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.