-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Replace ContainerRequest.BindMounts and ContainerRequest.VolumeMounts with ContainerRequest.Mounts as dedicated type #386
Conversation
mounts.go
Outdated
package testcontainers | ||
|
||
import ( | ||
"github.com/docker/docker/api/types/mount" |
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.
As I understood the already existing code the things related to particular container backend (in this case - docker) are kept in the docker.go
so that the testcontainers implementation is decoupled from it (although there is right now only one container backend provider).
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.
I see. To be honest after this comment I assumed it would be okay to rely on Docker API types but I refactored my draft to be Docker API independent, moved all the Docker related code to mounts_docker.go
and split the mount sources into generic, container provider agnostic variants and added Docker specific ones in case one would like to be able to provide e.g. volume options.
I hope this matches better the current code structure?
@mdelapenya probably any feedback on this? 😊 |
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.
Hey @baez90, thanks for this great PR, I added a few comments, although I'm OK with the changes. Let'd discuss them before moving forward
Thanks in advance and sorry for the late response, I was on PTO past week
mounts_docker.go
Outdated
@@ -0,0 +1,116 @@ | |||
package testcontainers |
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.
NIT: I'd rename this file to docker_mounts.go
as it's used as ..DockerMounts...
everywhere.
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.
fair enough!
mounts_docker.go
Outdated
|
||
// MapToDockerMounts maps the given []ContainerMount to the corresponding | ||
// []mount.Mount for further processing | ||
func MapToDockerMounts(containerMounts ContainerMounts) []mount.Mount { |
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.
Do we want to expose this method externally? Not against it, but I'm simply double-checking it's on purpose
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.
I tend to expose rather more than less because personally I prefer a breaking API instead of an API I cannot easily extend or 'hack' but for the sake of stability I made it private.
I was actually already wondering about the same and I think if there is a use case it can still be done later on?
mounts_docker.go
Outdated
MountTypeBind: mount.TypeBind, | ||
MountTypeVolume: mount.TypeVolume, | ||
MountTypeTmpfs: mount.TypeTmpfs, | ||
MountTypePipe: mount.TypeNamedPipe, |
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.
I cannot see an implementation for MountTypePipe
but there are for Bind, Volume and Tmpfs. Is this on purpose?
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.
😄 yes and no, I don't have any Windows running around here and I could not think of good test for MountTypePipe
without it but it is supported for Docker (although I don't expect it to be implemented e.g. in Podman) hence I thought if someone has the need for it, it can be done without having to change testcontainers-go
because it's already supported in the mapping.
There's no such thing like mount.NamedPipeOptions
in Docker so no need to support it with an optional interface. I can remove it, it's another 'keep it open'-thing I thought could be nice
1432c73
to
91dc436
Compare
Thanks @mdelapenya for your kind words and the feedback! I tried to 'resolve' everything you mentioned. |
Codecov Report
@@ Coverage Diff @@
## master #386 +/- ##
==========================================
+ Coverage 63.27% 64.16% +0.89%
==========================================
Files 16 18 +2
Lines 1070 1108 +38
==========================================
+ Hits 677 711 +34
- Misses 291 293 +2
- Partials 102 104 +2
Continue to review full report at Codecov.
|
- introduce specific types for different mount sources - support read-only mount - introduce convenience methods to support common mount scenarios - add validation to avoid duplicate mount targets
- introduce Docker specific mount sources and generic counterparts - move Docker specific code to mounts_docker.go
91dc436
to
57d25b2
Compare
c9b9cb2
to
acb1983
Compare
Can we get a note about a migration for this breaking change? I would like to place it at the top of this PR and in the changelog! Let's get this bc break ready to get merged! |
Sure thing! I tried to improve the title and added some examples how to migrate in the PR description. |
Thanks a lot for your help with this! |
What does this PR do?
Why is it important?
The current map based data structure is somehow confusing. This change avoids confusion by introducing dedicated types for all components to get help by IDEs and the compiler.
How to migrate
Simple bind mount
becomes:
Multiple mounts including a volume mount
becomes