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

feat: add image-keep option for built images #1785

Merged
merged 5 commits into from
Oct 26, 2023

Conversation

lefinal
Copy link
Contributor

@lefinal lefinal commented Oct 19, 2023

What does this PR do?

Adds an option to keep images if they have been built.
This avoids the need to build these images every time from scratch.
Built images are being kept in DockerContainer.Terminate when KeepImage is set to true in ContainerRequest.

Why is it important?

If images for testing are built with long build time, they are currently always deleted.
This means that for each test run, the whole image needs to be rebuilt.
If the image stays the same for every test, this may take up a lot of time.

Related issues

How to test this PR

Create a container with image-build manually and set KeepImage to true.
Tests for this functionality have been added as well.

Adds an option to keep images if they have been built.
This avoids the need to build these images everytime from scratch.
@lefinal lefinal requested a review from a team as a code owner October 19, 2023 17:27
@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 2b1fc56
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/653a7b369a2b5d0008128ffc
😎 Deploy Preview https://deploy-preview-1785--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.

container.go Outdated Show resolved Hide resolved
container.go Outdated Show resolved Hide resolved
docker_test.go Outdated Show resolved Hide resolved
Copy link
Member

@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.

I added a few comments that need to be addressed. But other than that, this LGTM.

Once resolved I think we can merge this one, thanks for your contribution!

@mdelapenya
Copy link
Member

@lefinal I'm planning a release hopefully this week, so I took this PR and fixed the lint for you (running make lint from root of the project).

Regarding my comments regarding coding style, are you interested in updating the PR with them? I can do it myself on top of your commits in order to have this PR in the new release

docker.go Show resolved Hide resolved
docker_test.go Outdated Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Oct 26, 2023
@mdelapenya mdelapenya added the enhancement New feature or request label Oct 26, 2023
@lefinal
Copy link
Contributor Author

lefinal commented Oct 26, 2023

@lefinal I'm planning a release hopefully this week, so I took this PR and fixed the lint for you (running make lint from root of the project).

Regarding my comments regarding coding style, are you interested in updating the PR with them? I can do it myself on top of your commits in order to have this PR in the new release

I made the requested changes. Do you want me to squash them so that we have a clean history? Or are the 5 commits okay. Don't know if you generate any changelogs from them.

@mdelapenya
Copy link
Member

I made the requested changes. Do you want me to squash them so that we have a clean history? Or are the 5 commits okay. Don't know if you generate any changelogs from them.

Thanks for doing it! No worries, we always squash the PRs when merging them into main. Will trigger the CI (it will pass, I ran your tests locally and they pass) and merge the PR.

Copy link
Member

@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.

LGTM! Thanks for taking the time to contribute this feature.

@mdelapenya mdelapenya merged commit 636c3cb into testcontainers:main Oct 26, 2023
112 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants