Skip to content

Add volume subpath option - #594

Merged
ndeloof merged 1 commit into
compose-spec:mainfrom
vvoland:volume-subpath
Mar 14, 2024
Merged

ndeloof merged 1 commit into
compose-spec:mainfrom
vvoland:volume-subpath

Conversation

@vvoland

@vvoland vvoland commented Mar 11, 2024

Copy link
Copy Markdown
Contributor

Only supported in the long syntax.

@vvoland
vvoland requested a review from ndeloof as a code owner March 11, 2024 11:06
Comment thread format/volume.go Outdated
volume.ReadOnly = false
case "nocopy":
volume.Volume = &types.ServiceVolumeVolume{NoCopy: true}
case "subpath":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

that sounds a bit weird to me: if we can detect use of subpath here, why not support this syntax ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason is to make it consistent with the Docker CLI where we don't support the short syntax, only the long --mount.
We decided to not support it in the short syntax is to avoid growing parsing complexity.

But if compose is fine with that, I'm happy to implement it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

right, then maybe better have a generic default case rejecting any unsupported attribute

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The default branch seems to explicitly ignore the unsupported attributes:

default:
if isBindOption(option) {
setBindOption(volume, option)
}
// ignore unknown options

Also, in this case the option is supported, just not in the short syntax. Returning an "not supported" message could make the user think that it's not supported at all.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

how does the CLI report attempt to use such an attribute ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CLI errors out when an unknown attribute is used in both long and short syntax.

As for the subpath option specifically, it's not handled differently and will error out just the same as an invalid attribute.

$ docker run -v asddd:/v:volume-subpath=sub --rm -it alpine                                                                                                                                                                      
docker: Error response from daemon: invalid mode: volume-subpath=sub.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

right, then I don't think compose should try to be more clever/friendly here OR subpath should be made an explicit short-syntax feature

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. Removed the special case for the short syntax.

Only supported in the long syntax.

Signed-off-by: Paweł Gronowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants