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

Improve usability of reusable containers #2416

Closed

Conversation

aguibert
Copy link
Contributor

Currently in order to take advantage of the reusable containers feature, two things must happen:

  1. The container must explicitly declare withReuse(true)
  2. The runtime environment must have testcontainers.reuse.enable=true set in ~/.testcontainers.properties

Item (1) is pretty obvious and straightforward, but (2) is a pain point, or something that a lot of people don't know about or don't notice (especially if they weren't the ones that initially added withReuse(true) to the codebase. It basically depends on per-project setup documentation (which many devs never read if they've already been working on the codebase for a while), or word-of-mouth among devs collaborating on a project.

The reason why (2) is needed is to prevent reusable containers from hanging around in CI environments -- we only want reusable containers to really be active for developer workstations.

I propose that we use logic found in the NodeJS ci-info library to automatically detect the presence of CI. If the presence of CI is not detected, we can enable reusable containers purely based on (1) which eliminates the manual step of each dev working on a project doing (2).

This approach works for the following CI servers:

  • Travis CI
  • Circle CI
  • Jenkins
  • Cirrus CI
  • Gitlab CI
  • Appveyor
  • Codeship
  • TeamCity
  • TaskCluster
  • dsari

which is just as many (if not more) CI environments than Testcontainers itself is supported in. If we find more in the future, we can always add them later.

|| !isCI();
}

private boolean isCI() {
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid we cannot use this approach. While it does support some percentage of environments, it is too easy to break it:

$ docker run -it --rm -v /var/run/docker.sock:/var/run/docker.sock maven:3 ./mvnw test

The feature is powerful but dangerous, this is why we decided to make it opt-in, not opt-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pardon my ignorance but what do you mean by saying it is too easy to break it <docker cmd>? From looking at it I'm not sure why that command would be an issue. Is it because if a developer runs a maven build inside a docker container locally the CI env vars wouldn't be set and therefore containers would get leftover?

Copy link
Member

Choose a reason for hiding this comment

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

it is a common way of running tests inside a container on CIs.
Docker won't pass host's environment variables, leading to reuse being activated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks for explaining that. I didn't realize that was a common pattern (docker-in-docker-in-docker) but it would certainly be an issue. I'll go ahead and close out this PR since the approach won't work.

Copy link
Member

Choose a reason for hiding this comment

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

@aguibert maybe you can keep it for the log.warn? It still makes sense to improve that message 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I had completely forgotten about that! Let me re-open and include just that change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened a separate PR here: #2422

@bsideup
Copy link
Member

bsideup commented Mar 10, 2020

@aguibert thanks for working on it!

While I understand the UX problem, I am deeply concerned about enabling it implicitly. In fact, as a developer, I would not want tests to keep any leftovers unless I explicitly let them do so. Especially given that the feature isn't ready yet and the leftovers will stay forever and require a manual action.

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.

None yet

2 participants