testcontainers-java icon indicating copy to clipboard operation
testcontainers-java copied to clipboard

Fix 3238 build args in from statement

Open Donnerbart opened this issue 2 years ago • 3 comments

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.

Donnerbart avatar Nov 03 '22 12:11 Donnerbart

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.

Donnerbart avatar Feb 22 '23 08:02 Donnerbart

Hey folks, is there any chance to get this merged? We still have a workaround in our code base to mitigate this bug :(

Donnerbart avatar Aug 01 '23 16:08 Donnerbart

Hey folks, can we please move this forward somehow? #3238 is still an issue for us.

Donnerbart avatar Oct 04 '23 10:10 Donnerbart

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.

eddumelendez avatar Jul 17 '24 04:07 eddumelendez

Thanks for your contribution, @Donnerbart !

eddumelendez avatar Jul 17 '24 05:07 eddumelendez

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 value false. 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.

image

Donnerbart avatar Jul 17 '24 08:07 Donnerbart