fix subpath in ServiceVolumeConfig#String #688
Conversation
ndeloof
changed the title
|
As docker run -v rejects unknown bind options, maybe better just remove |
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
| if s.Volume != nil && s.Volume.Subpath != "" { | ||
| options = append(options, s.Volume.Subpath) | ||
| } |
There was a problem hiding this comment.
Just to be sure, we have a warning in Compose to let the users know it won't be used?
What adding the warning here for all the other implementations?
There was a problem hiding this comment.
nope, Docker Compose HAS to use the mount API when SubPath or CreateHostPath is set
There was a problem hiding this comment.
🤦♂️ Ok I wasn't concentrated enough when doing my review
| @@ -95,7 +95,7 @@ func populateFieldFromBuffer(char rune, buffer []rune, volume *types.ServiceVolu | |||
| if isBindOption(option) { | |||
| setBindOption(volume, option) | |||
| } | |||
| // ignore unknown options | |||
| // ignore unknown options FIXME why not report an error here? | |||
There was a problem hiding this comment.
I would vote to add a message here since the engine error message becomes cryptic to the user since these modes are set by compose so the engine can do its thing
There was a problem hiding this comment.
The engine has it's own parser for the bind string, which reject unknown options.
Docker Compose just must not send a bind string to engine when it can't translate into, but String can't return an error
quick fix as subpath option is malformed
To be debated: as this option is actually not supported in the string format, why not just exclude it
or why don't we also include createHostPath then ? #inconsistency