Implement pre-flight checks#363
Conversation
|
Added CHANGELOG record :) Ready for the merge from my side |
| private String vncRecordedContainerImage = "richnorth/vnc-recorder:latest"; | ||
| private String tinyImage = "alpine:3.2"; | ||
| private String tinyImage = "alpine:3.5"; | ||
| private Boolean disableChecks = false; |
There was a problem hiding this comment.
Curios here: Why Boolean and not boolean?
There was a problem hiding this comment.
leftover from previous implementation actually :) will change to boolean, thanks :)
kiview
left a comment
There was a problem hiding this comment.
It seems configuring Testcontainers using properties isn't documented yet, we might want to add this now as well.
| if (!TestcontainersConfiguration.getInstance().getDisableChecks()) { | ||
| VisibleAssertions.info("Checking the system..."); | ||
|
|
||
| VisibleAssertions.assertThat("Docker version", version.getVersion(), new BaseMatcher<String>() { |
There was a problem hiding this comment.
Do you think it would be possible, to have each check in its own method? Would make the method much more readable and it would directly be visible, which checks are performed.
There was a problem hiding this comment.
Agree re putting each check in a separate method.
rnorth
left a comment
There was a problem hiding this comment.
Only trivial comments from me. Feel free to merge when you're happy with it!
| public void describeTo(Description description) { | ||
| description.appendText("is newer than 1.6.0"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Couldn't this be a bit shorter by just using an assertTrue method? An anonymous inner class to satisfy assertThat feels a bit heavy..?
There was a problem hiding this comment.
I decided to use Matcher because in case of a failure it will print current version - helps to debug
| if (!TestcontainersConfiguration.getInstance().getDisableChecks()) { | ||
| VisibleAssertions.info("Checking the system..."); | ||
|
|
||
| VisibleAssertions.assertThat("Docker version", version.getVersion(), new BaseMatcher<String>() { |
There was a problem hiding this comment.
Agree re putting each check in a separate method.
To simplify the first experience with TestContainers, we can provide pre-flight checks to distinguish user errors and environment/Docker/TestContainers errors.
Implemented checks:
The overhead of running these additional checks (compared to what we already had before this change, ~3s on my machine) is very small (100-200ms on my machine), but I also added a property to disable the checks completely (imagine
experienced users who already integrated TC and want to speed up their local setup for instance)