refactor(cli/compose/loader): extract ParseVolume() to its own package #6202
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
cli/command/container/opts.go
Outdated
| // add any bind targets to the list of container volumes | ||
| for bind := range copts.volumes.GetMap() { | ||
| parsed, err := loader.ParseVolume(bind) | ||
| parsed, err := parsevolume.ParseVolume(bind) |
There was a problem hiding this comment.
As we're moving this internal, we could take the opportunity to rename, e.g. volumespec for the package, and making this volumespec.Parse().
internal/parsevolume/parsevolume.go
Outdated
| @@ -1,4 +1,4 @@ | |||
| package loader | |||
| package parsevolume | |||
There was a problem hiding this comment.
We should probably move the related types as well, then alias the ones in the old location; this keeps backward-compatibility in case anyone currently imports cli/compose/loader and who may be using the things we moved.
Go does allow aliasing types that are internal, which means we can keep the implementation internal, and still keep the existing place. (for now at least). If we'd ever be working on re-implementing, we could split the two (as command/container only uses it internally.
|
Also opened a temporary draft PR rebased on current master to run CI |
| func ParseVolume(spec string) (types.ServiceVolumeConfig, error) { | ||
| volume := types.ServiceVolumeConfig{} | ||
| // Parse parses a volume spec without any knowledge of the target platform | ||
| func Parse(spec string) (VolumeConfig, error) { |
There was a problem hiding this comment.
IMHO this should be part of the moby API, so that Compose doesn't need a copy of this func in https://github.com/compose-spec/compose-go/blob/6d8281ca46429b18d5fb6e1a1f94bcd11489c578/format/volume.go#L32
There was a problem hiding this comment.
Yes; TBD where the best place would be, and this is definitely not the final state. There's currently already at least two (or three, if you add the fork in compose) places where this is maintained, which is horrible because some parts of it are definitely non-trivial.
I recall some discussion happened at the time when this parsing was added from the "libcompose" rewrite in Go (moby/moby#27998), which got later rewritten again, and ironically, #152 was done because there lived 3 separate implementations in the CLI, but there's also a daemon-side package to handle parsing, but it's neither part of the API, nor of the Client;
https://github.com/moby/moby/blob/fcb916ad17311aec838e03b47f93bfc70639fdf5/daemon/volume/mounts/parser.go#L28-L50
But there's people breathing down my neck to "ship the modules" and it's much easier to add things than to remove things (or break things) if we want the modules to remain SemVer, so this is just an intermediate step; likely to be (re)unified after a cleanup of these code-bases.
thaJeztah
added
status/2-code-review
kind/refactor
Moves ParseVolume() to a new internal package to remove the dependency on cli/compose/loader in cli/command/container/opts.go refactor to keep types isolated - rename the package to "volumespec" to reuse the name of the package as part of the name (parsevolume.ParseVolume() -> volumespec.Parse()) - move the related compose types to the internal package as well, and rename them to be more generic (not associated with "compose"); - ServiceVolumeConfig -> VolumeConfig - ServiceVolumeBind -> BindOpts - ServiceVolumeVolume -> VolumeOpts - ServiceVolumeImage -> ImageOpts - ServiceVolumeTmpfs -> TmpFsOpts - ServiceVolumeCluster -> ClusterOpts - alias the internal types inside cli/compose/types to keep backward compatibility (for any external consumers); even though the implementation is internal, Go allows aliasing types to use them externally. Signed-off-by: Michael Tews <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM let me get this one in 😄
4 participants
Moves
ParseVolume()to a new internal package to remove the dependency oncli/compose/loaderincli/command/container/opts.gofixes #6188
- What I did
Removed the
cli/compose/loaderdependency fromcli/command/container.- How I did it
Moved the
cli/compose/loader.ParseVolumefunction to a new internal package namedparsevolume.- How to verify it
Verify that the
cli/command/containerpackage does not importcli/compose/loader.- Human readable description for the release notes