Feature: Add generic option to docker image#23250
Feature: Add generic option to docker image#23250tim-werner wants to merge 8 commits intopantsbuild:mainfrom
Conversation
tim-werner
force-pushed
the
add-generic-option-to-docker-image
branch
from
124c44c to
99f5e5d
Compare
There was a problem hiding this comment.
Hi! Thanks for the contribution!
The feature looks well thought out w.r.t how options should be propagated, and I agree with the general direction of providing a list of args instead of adding a new option for each arg.
I am however not going to review the test cases before you do. Happy to once they at least conform to the style of the test file where they were introduced.
| """Return the set of flag names (e.g. ``--pull``) present in *extra_options*. | ||
|
|
||
| Handles both ``--flag=value`` and ``--flag value`` styles so that any structured field whose | ||
| ``docker_build_option`` class variable matches one of these names will be suppressed in favour | ||
| of the caller-supplied value, avoiding duplicate or conflicting flags on the command line. |
There was a problem hiding this comment.
This reads like LLM output. The function name and readable python code serves the same purpose as this very verbose docstring. There is also no logic here that enforces the caller-supplied override.
| filtered_global = tuple( | ||
| opt | ||
| for opt in global_extra_options | ||
| if opt.startswith("-") and opt.split("=")[0] not in target_flag_names |
There was a problem hiding this comment.
The logic of extracting a flag name repeats for the per-target set. I also suggest extract a function for the option name collision logic to spare us the continous growth of get_build_options.
| flag = getattr( | ||
| field_type, "docker_build_option", None | ||
| ) # get the flag name if it exists such as --pull or --network, etc. | ||
| if flag and flag in overridden_flags: | ||
| continue # skip this field since its flag is already covered by extra_options |
|
|
||
|
|
||
| def test_global_build_extra_options(rule_runner: RuleRunner) -> None: | ||
| """Global build_extra_options from [docker] should be passed to the docker build command.""" |
There was a problem hiding this comment.
LLM docstrings that you have not vetted (the example I am commenting is obviously redundant considering the test name and assertion).
Please re-review and cleanup your test cases and LMK when you have and I'll continue review! 👍
|
Hi @tobni , |
sureshjoshi
left a comment
sureshjoshi
left a comment
There was a problem hiding this comment.
Thanks for the contribution. We've just branched for 2.32.x, so merging this pull request now will come out in 2.33.x, please move the release notes updates to docs/notes/2.33.x.md if that's appropriate.
3 participants
This pull request allows it now to add generic build options to docker/podman build. It will also work as a workaround for issue: #21450
It is now possible to use pass generic build options to docker and podman. The old arguments such as pull on docker_image target level or no-cache on global level will still work, however the new build_extra_option will have precedence.
AI was used to find the right locations in the code base for the changes and to write the tests faster. However, most of the code was written by human.
This is an improvement of pull request: #23032