testcontainers-java
testcontainers-java copied to clipboard
Fix 3238 build args in from statement
Resolves #3238 Closes #3922
Since #3922 has merge conflicts, the author is inactive on GitHub for over a year and the provided solution is missing some resolve cases, I've create a new PR which also cleans up some warnings and deprecated code usage in the involved classes.
Hey folks :) Is there anything I can do to move this forward?
Beside the small cleanups and replacement of deprecated API, the actual code for the fix comes from com.github.dockerjava.core.dockerfile.DockerfileStatement
, so it's a battle proven solution. So I hope this PR is easy to review.
Hey folks, is there any chance to get this merged? We still have a workaround in our code base to mitigate this bug :(
Hey folks, can we please move this forward somehow? #3238 is still an issue for us.
Thanks for the contribution, @Donnerbart! Sorry for the delay reviewing this. I've reverted part of the cleanup done in bcd37abff307415aead91472ba784bda56b611db. One of the changes switch the deleteOnExit
to the default value false
. I'm not sure if this is intended.
Thanks for your contribution, @Donnerbart !
Thanks for the contribution, @Donnerbart! Sorry for the delay reviewing this. I've reverted part of the cleanup done in bcd37ab. One of the changes switch the
deleteOnExit
to the default valuefalse
. I'm not sure if this is intended.
Thanks for the review and merge.
I think you have misread the code here. The actual value of deleteOnExit
was and is always set from the constructors. And there we set it to true
if not set otherwise.
There was no need to define a value in the field declaration and have it non-final, just to set it again in the constructor. It's just misleading to have the value set in two places (and have a non-final field for a value that is never changed again). That's exactly the reason why I've changed it.
EDIT: IntelliJ even shows a warning for this.