-
-
Notifications
You must be signed in to change notification settings - Fork 513
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
Swap incorrect key and value of BindMounts and VolumeMounts #354
Conversation
It is currently not possible to mount a local path to multiple paths in the container. BindMounts are currently stored in a `map[string]string` with the hostPath being the key and containerPath as the value. This prevents me from mounting a hostPath to multiple paths in the container. Using a map actually makes sense because it is not possible to mount multiple host paths to the same path in the container. Key and value are just swapped. Same with VolumeMounts. Yes, i know, this a huge breaking change and probably not the best fix. We could also add an additional map like `BindMountsFixed` and deprecate the old one. But as this library is still in version 0, a breaking change might not be a problem?
Codecov Report
@@ Coverage Diff @@
## master #354 +/- ##
==========================================
+ Coverage 61.91% 62.11% +0.19%
==========================================
Files 15 15
Lines 1011 1011
==========================================
+ Hits 626 628 +2
+ Misses 288 286 -2
Partials 97 97
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Lucaber this LGTM, thanks for your contribution!
I'm ok with the implications on these breaking changes, as I understand your user case, but I'd like to listen to the community first before merging and releasing a new version.
Please give us a few more time for that 🙏
@gianarb what are your thoughts on this?
Can we get this merged? @mdelapenya @gianarb |
@mdelapenya @gianarb any updates on this? :) |
Thanks! @bsideup do you mind to tweet about this bc break? Thanks! |
Hi @gianarb! I would be happy to retweet, but I am not sure I need to be the one tweeting, or at least I don't see why 😅 |
It is currently not possible to mount a local path to multiple paths in
the container.
BindMounts are currently stored in a
map[string]string
with thehostPath being the key and containerPath as the value.
This prevents me from mounting a hostPath to multiple paths in the
container. Using a map actually makes sense because it is not possible
to mount multiple host paths to the same path in the container. Key and
value are just swapped. Same with VolumeMounts.
Yes, i know, this a huge breaking change and probably not the best fix.
We could also add an additional map like
BindMountsFixed
and deprecatethe old one. But as this library is still in version 0, a breaking
change might not be a problem?