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

Show volume options for `docker volume inspect` #26671

Merged

Conversation

@yongtang
Copy link
Member

commented Sep 17, 2016

- What I did

This fix tries to address the issue raised in #25545 where volume options at the creation time is not showed up in docker volume inspect.

- How I did it

This fix adds the field Options in Volume type and persist the options in volume db so that volume inspect could display the options.

- How to verify it

This fix adds a couple of test cases to cover the changes.

- Description for the changelog

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

This fix fixes #25545
This fix fixes #27493

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

Thanks! This will need docs changes once we're past code review.

Also (after this PR), now that we store actual options, we may be able to better detect conflicting options, and address #16068

ping @cpuguy83

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2016

Design OK.

}

func (v volumeWrapper) Options() map[string]string {
return v.options

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Sep 18, 2016

Contributor

This should return a copy of the map instead of the actual map.

This comment has been minimized.

Copy link
@yongtang

yongtang Sep 19, 2016

Author Member

Done.

@@ -73,6 +73,12 @@ type LabeledVolume interface {
Volume
}

// OptionedVolume wraps a Volume with driver defined options
type OptionedVolume interface {

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Sep 18, 2016

Contributor

Maybe we can replace all these 1-off interfaces with a single VolumeWithDetails... maybe a better name.
But all volumes will have it since we are wrapping all the volumes.

This comment has been minimized.

Copy link
@yongtang

yongtang Sep 19, 2016

Author Member

Thanks @cpuguy83. I updated the PR and use VolumeWithDetails for now. Let me know if other names are preferred.

Update: Have to change the VolumeWithDetails to DetailedVolume as otherwise golint will complain.

This comment has been minimized.

Copy link
@yongtang

yongtang Sep 19, 2016

Author Member

@cpuguy83 The VolumeWithDetails has been changed to DetailedVolume as otherwise golint will complain.

@@ -476,7 +490,7 @@ func (s *VolumeStore) FilterByDriver(name string) ([]volume.Volume, error) {
return nil, &OpErr{Err: err, Name: name, Op: "list"}
}
for i, v := range ls {
ls[i] = volumeWrapper{v, s.labels[v.Name()], vd.Scope()}
ls[i] = volumeWrapper{v, s.labels[v.Name()], vd.Scope(), s.options[v.Name()]}

This comment has been minimized.

Copy link
@cpuguy83

cpuguy83 Sep 18, 2016

Contributor

Should be a copy and not the actual map.

This comment has been minimized.

Copy link
@yongtang

yongtang Sep 19, 2016

Author Member

Done.

@yongtang yongtang force-pushed the yongtang:25545-docker-volume-inspect-volume-options branch 3 times, most recently from 590a4e7 to 737b6d4 Sep 19, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2016

Thanks @cpuguy83 for the review. The PR has been updated. Please let me know if there are additional issues.

Thanks @thaJeztah for the reminder. I will update the docs once code review is done. Will also take a look at #16068 and see what we could do with it.

@yongtang yongtang force-pushed the yongtang:25545-docker-volume-inspect-volume-options branch from 737b6d4 to 4824d3a Sep 19, 2016

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

LGTM

@yongtang yongtang force-pushed the yongtang:25545-docker-volume-inspect-volume-options branch from 4824d3a to 34e478b Oct 3, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2016

@thaJeztah This PR has been rebased with docs updated. Please take a look and let me know if there are any issues. (Will try to have another PR for #16068 later)

@yongtang yongtang force-pushed the yongtang:25545-docker-volume-inspect-volume-options branch from 34e478b to 8fd9ff2 Oct 18, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2016

Rebased the PR to fix the merge conflict. Also updated the description to include fixes #27493 (See #27493).

@@ -447,6 +447,7 @@ type Volume struct {
Status map[string]interface{} `json:",omitempty"` // Status provides low-level status information about the volume
Labels map[string]string // Labels is metadata specific to the volume
Scope string // Scope describes the level at which the volume exists (e.g. `global` for cluster-wide or `local` for machine level)
Options map[string]string // Options holds the driver specifi options to use for when creating the volume.

This comment has been minimized.

Copy link
@allencloud

allencloud Oct 18, 2016

Contributor

specifi, Typo?

This comment has been minimized.

Copy link
@yongtang

yongtang Oct 19, 2016

Author Member

Thanks. Done.

"Scope": "local",
"Options":{
"some-key": "some-value",
"some-other-key": "some-other-value"

This comment has been minimized.

Copy link
@allencloud

allencloud Oct 18, 2016

Contributor

If the driver is local, I think only three kinds of options are valid, type, o, device. If that is true, maybe use that will be better.

This comment has been minimized.

Copy link
@yongtang

yongtang Oct 19, 2016

Author Member

Thanks @allencloud. The PR has been updated with the doc changes.

for key, value := range v.options {
options[key] = value
}
return options

This comment has been minimized.

Copy link
@allencloud

allencloud Oct 18, 2016

Contributor

Can it just return v.options?

This comment has been minimized.

Copy link
@yongtang

yongtang Oct 19, 2016

Author Member

Thanks @allencloud . This is based on the other comment to return the copy here.

This comment has been minimized.

Copy link
@allencloud

This comment has been minimized.

Copy link
@allencloud

allencloud Oct 20, 2016

Contributor

Thanks @yongtang. I know that deep copy is correct. While I still can not follow why options needs a deep copy, while labels does not. @cpuguy83 Can you give me some explaination. Thanks a lot.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Oct 20, 2016

Member

It would probably make sense for labels as well, but that's out of scope for this PR

@yongtang yongtang force-pushed the yongtang:25545-docker-volume-inspect-volume-options branch from 8fd9ff2 to b122a37 Oct 19, 2016

@thaJeztah
Copy link
Member

left a comment

LGTM

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

ping @vdemeester PTAL

@vdemeester
Copy link
Member

left a comment

LGTM 🐸

@vdemeester

This comment has been minimized.

Copy link
Member

commented Oct 20, 2016

@yongtang needs a rebase though 👼

Show volume options for `docker volume inspect`
This fix tries to address the issue raised in 25545 where
volume options at the creation time is not showed up
in `docker volume inspect`.

This fix adds the field `Options` in `Volume` type and
persist the options in volume db so that `volume inspect`
could display the options.

This fix adds a couple of test cases to cover the changes.

This fix fixes 25545.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

@yongtang yongtang force-pushed the yongtang:25545-docker-volume-inspect-volume-options branch from b122a37 to 9ce8aac Oct 20, 2016

@yongtang

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2016

Thanks all for the review! The PR has been rebased and all tests passed. Ping @vdemeester.

@vdemeester vdemeester merged commit 78624ec into moby:master Oct 20, 2016

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25149 has succeeded
Details
janky Jenkins build Docker-PRs 33746 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 4601 has succeeded
Details

@yongtang yongtang deleted the yongtang:25545-docker-volume-inspect-volume-options branch Oct 20, 2016

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.