-
-
Notifications
You must be signed in to change notification settings - Fork 678
Fix #22602: targets in extra_build_args #22603
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
base: main
Are you sure you want to change the base?
Conversation
|
I haven't forgotten about this, will try and get to it this weekend. |
|
Meanwhile the CI test failures look meaningful. |
benjyw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Having paged some of this backend into my brain, this seems right. But see my question, which will help me understand this better.
| # Override default value type for the `build_args` context to get helpful error messages. | ||
| interpolation_context["build_args"] = DockerBuildArgsInterpolationValue( | ||
| interpolation_context.get("build_args", {}) | ||
| merged_build_args_with_extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is quite subtle, so help me understand something...
This seems to be the critical line, where it's important for some reason that merged_build_args_with_extensions is not converted to a DockerBuildArgsInterpolationValue. Why does this fix the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Candidly confessing I asked Claude Code :)
The Problem:
When InterpolationContext.from_dict(interpolation_context) is called on line 168, it processes all values through the create_value() method from value_interpolation.py:45-49:
def create_value(value: str | Mapping[str, str]) -> str | InterpolationValue:
if isinstance(value, (str, InterpolationValue)):
return value # Already wrapped, keep as-is
return InterpolationValue(value) # Plain dict, wrap in generic InterpolationValue
The Fix:
Lines 145-147: Wraps the dict in DockerBuildArgsInterpolationValue and stores it in the interpolation context
Line 158: Passes the unwrapped dict to _get_stages_and_tags() (which only needs the raw data)
Line 168: When from_dict() processes the interpolation context, it checks each value with isinstance(value, InterpolationValue).
Since DockerBuildArgsInterpolationValue is already an InterpolationValue subclass, it's preserved as-is rather than being re-wrapped in a generic InterpolationValue
Why it matters:
DockerBuildArgsInterpolationValue provides custom error messages via its _attribute_error_type, which helps users understand missing build args.
If it got replaced by a generic InterpolationValue, users would get less helpful error messages.
The subtlety
Passing the unwrapped dict to line 146 (instead of the DockerBuildArgsInterpolationValue wrapper) ensures the wrapper survives the from_dict() call on line 168, because from_dict() only checks isinstance(value, InterpolationValue) and preserves objects that are already InterpolationValue instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that answers my question... To clarify: How is merged_build_args_with_extensions different from interpolation_context["build_args"] and why does that matter?
| actual_dirs=snapshot.dirs, | ||
| ) | ||
| ) | ||
| if should_suggest_renames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous comment was that you appear to have stomped on the functionality that should_suggest_renames provides. Restoring it in the signature is not enough...
Was this change handwritten or claude code generated?
| # Override default value type for the `build_args` context to get helpful error messages. | ||
| interpolation_context["build_args"] = DockerBuildArgsInterpolationValue( | ||
| interpolation_context.get("build_args", {}) | ||
| merged_build_args_with_extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that answers my question... To clarify: How is merged_build_args_with_extensions different from interpolation_context["build_args"] and why does that matter?
Summary
extra_build_argsinBUILDfilesDockerBuildContext.create()to consistently use merged build args (includingextra_build_args) for both interpolation context andDockerfilestage parsingextra_build_argsoverride defaultARGvalues inDockerfilesProblem
Previously, when using target references like
:base-imageinextra_build_args, Pants failed to resolve these to actual Docker image tags during build context creation. This prevented users from creating hierarchical Docker image dependencies and dynamic base image selection.Solution
The fix ensures that
extra_build_argsfromBUILDfiles are properly merged and made available during all phases of build context creation, particularly when parsingDockerfilestages that containFROMinstructions with variable substitution.Test Coverage
Added
test_from_image_build_arg_with_extra_build_args()which validates:extra_build_argsextra_build_argsresolve to proper image tags