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

Support build args in FROM statement #6119

Conversation

Donnerbart
Copy link
Contributor

@Donnerbart Donnerbart commented Nov 3, 2022

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 Donnerbart requested a review from a team as a code owner November 3, 2022 12:39
@Donnerbart Donnerbart force-pushed the fix-3238-build-args-in-from-statement branch 3 times, most recently from c28968d to c646e7f Compare November 4, 2022 12:16
@Donnerbart Donnerbart force-pushed the fix-3238-build-args-in-from-statement branch from c646e7f to 5308ec7 Compare November 10, 2022 15:44
@Donnerbart Donnerbart force-pushed the fix-3238-build-args-in-from-statement branch 3 times, most recently from 20629cb to 0b94f24 Compare November 25, 2022 19:12
@Donnerbart Donnerbart force-pushed the fix-3238-build-args-in-from-statement branch 2 times, most recently from 8df3c95 to 39331de Compare December 25, 2022 15:41
@Donnerbart Donnerbart force-pushed the fix-3238-build-args-in-from-statement branch 3 times, most recently from ed5b8a1 to bb788f2 Compare January 16, 2023 11:44
@Donnerbart Donnerbart force-pushed the fix-3238-build-args-in-from-statement branch from bb788f2 to a4029fc Compare January 20, 2023 23:27
@Donnerbart
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Donnerbart
Copy link
Contributor Author

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

@eddumelendez
Copy link
Member

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.

@eddumelendez eddumelendez added this to the next milestone Jul 17, 2024
@eddumelendez eddumelendez changed the title Fix 3238 build args in from statement Support build args in FROM statement Jul 17, 2024
@eddumelendez eddumelendez merged commit 181b56e into testcontainers:main Jul 17, 2024
100 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @Donnerbart !

@Donnerbart
Copy link
Contributor Author

Donnerbart commented Jul 17, 2024

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 Donnerbart deleted the fix-3238-build-args-in-from-statement branch July 17, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to pre-fetch an image when using build-args in FROM
2 participants