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

chore: deprecate BindMount APIs #1907

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Nov 7, 2023

What does this PR do?

This PR deprecates all the BindMount types and functions/methods, removing their internal usage from the library (test code, basically).

The internal changes will modify the usage of BindMounts in this manner:

  • using ContainerRequest's Files field.
  • using ContainerRequest's HostCOnfigModifier field, using the closure to update the Binds of the Docker container's HostConfig type.
  • using the CopyFileToContainer/CopyDirToContainer methods of a running container.

It's also merging the docs regarding copy files and volumes, as both topics are closely related.

Why is it important?

BindMounts will make code not be portable cross docker environments, e.g. in remote Docker hosts, as the file binding won't exists in the remote.

For that reason we advocate for using the Files attribute of the container request in order to copy files/dirs to a container after it's created but before it's started, or the HostCOnfigModifier to define container binds, or CopyFileToContainer/CopyDirToContainer container methods.

Related issues

@mdelapenya mdelapenya requested a review from a team as a code owner November 7, 2023 20:21
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Nov 7, 2023
@mdelapenya mdelapenya self-assigned this Nov 7, 2023
Copy link

netlify bot commented Nov 7, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 7dd4b4e
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/654b4c5db1f0f90008e90dc7
😎 Deploy Preview https://deploy-preview-1907--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -305,6 +305,8 @@ func (c *ContainerRequest) validateContextOrImageIsSpecified() error {
return nil
}

// validateMounts ensures that the mounts do not have duplicate targets.
// It will check the Mounts and HostConfigModifier.Binds fields.
func (c *ContainerRequest) validateMounts() error {
Copy link
Collaborator Author

@mdelapenya mdelapenya Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think the validation functions are very little useful, as they actually do too few checks.

We probably need a follow up discussion to remove it or rethink what to check here

absPath, err := filepath.Abs(filepath.Join(".", "testdata", "hello.sh"))
if err != nil {
t.Fatal(err)
}
ctx, cnl := context.WithTimeout(context.Background(), 30*time.Second)
defer cnl()
// Create a Docker client.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to instantiate a client for the volume, as Docker will handle the mounts correctly.

@mdelapenya mdelapenya merged commit 555cb76 into testcontainers:main Nov 8, 2023
116 checks passed
@mdelapenya mdelapenya deleted the deprecate-mount-apis branch November 8, 2023 12:23
mdelapenya added a commit to kuisathaverat/testcontainers-go that referenced this pull request Nov 20, 2023
* main: (31 commits)
  feat: support for executing commands in a container with user, workDir and env (testcontainers#1914)
  fix(modules.kafka): Switch to MaxInt for 32-bit support (testcontainers#1923)
  docs: fix code snippet for image substitution (testcontainers#1918)
  Add database driver note to SQL Wait strategy docs (testcontainers#1916)
  Reduce flakiness in ClickHouse tests (testcontainers#1902)
  lint: enable nonamedreturns (testcontainers#1909)
  chore: deprecate BindMount APIs (testcontainers#1907)
  fix(reaper): fix race condition when reusing reapers (testcontainers#1904)
  feat: Allow the container working directory to be specified (testcontainers#1899)
  chore: make rabbitmq examples more readable (testcontainers#1905)
  chore(deps): bump github.com/twmb/franz-go and github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1896)
  Fix - respect ContainerCustomizer in neo4j module (testcontainers#1903)
  chore(deps): bump github.com/nats-io/nkeys and github.com/nats-io/nats.go in /modules/nats (testcontainers#1897)
  chore: add tests for withNetwork option (testcontainers#1894)
  chore(deps): bump google.golang.org/grpc and cloud.google.com/go/firestore in /modules/gcloud (testcontainers#1891)
  chore(deps): bump github.com/aws/aws-sdk-go and github.com/aws/aws-sdk-go-v2/config in /modules/localstack (testcontainers#1892)
  chore(deps): bump Github actions (testcontainers#1890)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.9 to 3.23.10 (testcontainers#1858)
  chore(deps): bump github.com/hashicorp/consul/api in /examples/consul (testcontainers#1863)
  chore(deps): bump github.com/IBM/sarama in /modules/kafka (testcontainers#1874)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Nov 30, 2023
* main: (100 commits)
  fix: fallback matching of registry authentication config (testcontainers#1927)
  feat: support customizing the Docker build command (testcontainers#1931)
  docs: include MongoDB's username and password options into the docs (testcontainers#1930)
  feat: support for custom registry prefixes at the configuration level (testcontainers#1928)
  Add username and password functions to mongodb (testcontainers#1910)
  chore: skip TestContainerLogWithErrClosed as flaky on rootless docker (testcontainers#1925)
  docs: add some Vault module examples (testcontainers#1825)
  feat: support for executing commands in a container with user, workDir and env (testcontainers#1914)
  fix(modules.kafka): Switch to MaxInt for 32-bit support (testcontainers#1923)
  docs: fix code snippet for image substitution (testcontainers#1918)
  Add database driver note to SQL Wait strategy docs (testcontainers#1916)
  Reduce flakiness in ClickHouse tests (testcontainers#1902)
  lint: enable nonamedreturns (testcontainers#1909)
  chore: deprecate BindMount APIs (testcontainers#1907)
  fix(reaper): fix race condition when reusing reapers (testcontainers#1904)
  feat: Allow the container working directory to be specified (testcontainers#1899)
  chore: make rabbitmq examples more readable (testcontainers#1905)
  chore(deps): bump github.com/twmb/franz-go and github.com/twmb/franz-go/pkg/kadm in /modules/redpanda (testcontainers#1896)
  Fix - respect ContainerCustomizer in neo4j module (testcontainers#1903)
  chore(deps): bump github.com/nats-io/nkeys and github.com/nats-io/nats.go in /modules/nats (testcontainers#1897)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant