Skip to content

Support build args in FROM statement#6119

Merged
eddumelendez merged 5 commits intotestcontainers:mainfrom
Donnerbart:fix-3238-build-args-in-from-statement
Jul 17, 2024
Merged

Support build args in FROM statement#6119
eddumelendez merged 5 commits intotestcontainers:mainfrom
Donnerbart:fix-3238-build-args-in-from-statement

Conversation

@Donnerbart
Copy link
Contributor

@Donnerbart Donnerbart commented Nov 3, 2022 •
edited
Loading

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 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

Donnerbart commented Feb 22, 2023

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

Donnerbart commented Aug 1, 2023

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

Donnerbart commented Oct 4, 2023

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

@Donnerbart Donnerbart mentioned this pull request Oct 4, 2023
Closed
@eddumelendez
Copy link
Member

eddumelendez 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.

@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 mentioned this pull request Jul 17, 2024
Closed
@eddumelendez eddumelendez merged commit 181b56e into testcontainers:main Jul 17, 2024
@eddumelendez
Copy link
Member

eddumelendez commented Jul 17, 2024

Thanks for your contribution, @Donnerbart !

@Donnerbart
Copy link
Contributor Author

Donnerbart commented Jul 17, 2024 •
edited
Loading

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
fokion pushed a commit to fokion/testcontainers-java that referenced this pull request Jan 18, 2025
@Donnerbart @eddumelendez @fokion
Support build args in FROM statement (testcontainers#6119)
d5273a3 
Fixes testcontainers#3238

---------

Co-authored-by: Eddú Meléndez Gonzales <eddu.melendez@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

type/enhancement

Projects

None yet

Milestone

1.20.0

Development

Successfully merging this pull request may close these issues.

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

2 participants

@Donnerbart @eddumelendez