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

Swap incorrect key and value of BindMounts and VolumeMounts #354

Merged
merged 1 commit into from Nov 19, 2021

Conversation

Lucaber
Copy link
Contributor

@Lucaber Lucaber commented Sep 14, 2021

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?

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
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #354 (3eed4e2) into master (19e857f) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
docker.go 65.13% <ø> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19e857f...3eed4e2. Read the comment docs.

Copy link
Collaborator

@mdelapenya mdelapenya left a 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?

@Lucaber
Copy link
Contributor Author

Lucaber commented Oct 11, 2021

Can we get this merged? @mdelapenya @gianarb

@gianarb gianarb added the breaking change Causing compatibility issues. label Oct 11, 2021
@thueske
Copy link

thueske commented Nov 2, 2021

@mdelapenya @gianarb any updates on this? :)

@gianarb gianarb merged commit 1517941 into testcontainers:master Nov 19, 2021
@gianarb
Copy link
Collaborator

gianarb commented Nov 19, 2021

Thanks! @bsideup do you mind to tweet about this bc break? Thanks!

@bsideup
Copy link
Member

bsideup commented Nov 19, 2021

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 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Causing compatibility issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants