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

Poor typing for BindMounts and VolumeMounts #384

Closed
prskr opened this issue Nov 24, 2021 · 3 comments
Closed

Poor typing for BindMounts and VolumeMounts #384

prskr opened this issue Nov 24, 2021 · 3 comments

Comments

@prskr
Copy link
Contributor

prskr commented Nov 24, 2021

After #354 the way of mounting volumes and host directories might be 'correcter' than before but neither the BindMounts nor the VolumeMounts field in ContainerRequest indicate in any way how to use them properly either by an example or an explanatory description and they behave now contrary to the CLI syntax most developers are used to.

I know there's a PR which 'fixes' the documentation issue already but I think when already breaking the API a typed solution might come in handy, also for #382 to support even more 'magical' things.

I propose to add the following types:

  • type ContainerMountSource interface
  • type HostMountSource struct{}
  • type VolumeMountSource struct{}
  • type ContainerMountTarget struct

the latter 3 could also be implemented as type ... string but I'd prefer to directly use structs e.g. to support multiple flags like readonly or even more advanced ones.

The fields BindMounts and VolumeMounts are then obsolete and a new Mounts map[ContainerMountTarget]ContainerMountSource or preferable Mounts []ContainerMounts could be introduced.

The latter slice based variant avoids confusion due to the current reversed 'target:src' syntax and internally it's still possible to convert the slice to a map to avoid duplicated targets and error before even starting the container.

If there's some interest in this I'd be glad to provide a PR but I thought I'd get some feedback in advance

@mdelapenya
Copy link
Collaborator

Thanks for raising this issue affecting usability and improving code performance.

Will be glad if reviewing it if you send a proposal

@prskr
Copy link
Contributor Author

prskr commented Nov 24, 2021

I implemented a first draft to illustrate what I mean and how it could look like when using it.

Also added some convenience methods. Could think of some more for different use cases.
Also parsing the different volume syntaxes could be implemented even though I'm not sure if there's a specific use case for it but that is up to you all?

@silh
Copy link

silh commented Dec 8, 2021

I do agree with better typing for mounts and the original implementation of #382. Will close my PR if this one is accepted.

@prskr prskr closed this as completed Dec 21, 2021
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

No branches or pull requests

3 participants