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

Read only bind and volume mounts #382

Closed
wants to merge 2 commits into from

Conversation

silh
Copy link

@silh silh commented Nov 17, 2021

Added the ability to specify read-only bind and volume mounts.
That ability is present in docker client itself as well as in java version of testcontainers - see https://www.testcontainers.org/features/files/

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #382 (61cfbb6) into master (1517941) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
- Coverage   62.28%   62.20%   -0.09%     
==========================================
  Files          15       15              
  Lines        1042     1045       +3     
==========================================
+ Hits          649      650       +1     
- Misses        291      292       +1     
- Partials      102      103       +1     
Impacted Files Coverage Δ
container.go 85.29% <ø> (ø)
docker.go 65.10% <100.00%> (+0.20%) ⬆️
wait/http.go 59.37% <0.00%> (-2.09%) ⬇️

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 1517941...61cfbb6. Read the comment docs.

@mdelapenya
Copy link
Member

Thanks for this contribution, it makes total sense.

I wonder whether directly merging it as a breaking change in next minor (we are still in 0.x.y) or include it in an eventual 1.0.0 release 🤔

@gianarb thoughts?

@silh
Copy link
Author

silh commented Nov 17, 2021

I though about making it non breaking but less obvious - keeping the map[string]string but parsing the end of the value to contain :ro suffix. Wasn't sure weather that's a good approach to take.

@gianarb
Copy link
Member

gianarb commented Nov 19, 2021

I would like to go with the :ro :rw parsing if possible! That syntax is stable since Docker showed up and I don't think it will change in the future 👍

@silh silh force-pushed the support-read-only-volumes branch from be9cc2b to 61cfbb6 Compare November 19, 2021 13:23
@silh
Copy link
Author

silh commented Nov 19, 2021

I would like to go with the :ro :rw parsing if possible! That syntax is stable since Docker showed up and I don't think it will change in the future 👍

Did it with :ro and `:rw: but while rebasing I saw that there was already breaking change which might create confusion with order of host path and container path in those maps and I think the structure would have been more clear.

@silh
Copy link
Author

silh commented Nov 24, 2021

Is anything else needed?
I am not sure about the coverage drop in wait/http.go - it seems flacky.

@mdelapenya
Copy link
Member

Can you double check if there are incompatible changes with #386?

@silh
Copy link
Author

silh commented Dec 20, 2021

I think #386 make this PR obsolete and it can be closed.
There is already a way there to specify if you want a readonly container.

@mdelapenya
Copy link
Member

Closed as per @silh's comment. Thanks you very much for your time with testcontainers-go!

image

@mdelapenya mdelapenya closed this Dec 21, 2021
@silh silh deleted the support-read-only-volumes branch December 21, 2021 10:16
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

Successfully merging this pull request may close these issues.

3 participants