-
-
Notifications
You must be signed in to change notification settings - Fork 694
Bugfix: Add support for pull option in podman #23032
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
Changes from all commits
60e9bfd
be23b67
cc856b1
38dcb98
dde9a9b
8eb88d9
53f4362
2c46fc4
ae42b18
11d42a3
c09ecdf
481ceca
4bf2e95
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,215 @@ | ||
| # Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
| # Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
|
||
| """Integration tests for Podman-specific pull policy behavior in DockerImageBuildPullOptionField.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from pants.backend.docker.goals.package_image import ( | ||
| DockerImageBuildProcess, | ||
| DockerImageRefs, | ||
| DockerPackageFieldSet, | ||
| ImageRefRegistry, | ||
| ImageRefTag, | ||
| get_docker_image_build_process, | ||
| rules, | ||
| ) | ||
| from pants.backend.docker.subsystems.docker_options import DockerOptions | ||
| from pants.backend.docker.target_types import DockerImageTarget | ||
| from pants.backend.docker.util_rules.docker_binary import DockerBinary | ||
| from pants.backend.docker.util_rules.docker_build_args import DockerBuildArgs | ||
| from pants.backend.docker.util_rules.docker_build_args import rules as build_args_rules | ||
| from pants.backend.docker.util_rules.docker_build_context import DockerBuildContext | ||
| from pants.backend.docker.util_rules.docker_build_env import DockerBuildEnvironment | ||
| from pants.backend.docker.util_rules.docker_build_env import rules as build_env_rules | ||
| from pants.engine.addresses import Address | ||
| from pants.engine.env_vars import EnvironmentVars | ||
| from pants.engine.fs import EMPTY_DIGEST | ||
| from pants.engine.target import InvalidFieldException, WrappedTarget | ||
| from pants.engine.unions import UnionMembership | ||
| from pants.testutil.option_util import create_subsystem | ||
| from pants.testutil.rule_runner import QueryRule, RuleRunner, run_rule_with_mocks | ||
| from pants.util.value_interpolation import InterpolationContext, InterpolationValue | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def rule_runner() -> RuleRunner: | ||
| return RuleRunner( | ||
| rules=[ | ||
| *rules(), | ||
| *build_args_rules(), | ||
| *build_env_rules(), | ||
| QueryRule(DockerOptions, []), | ||
| ], | ||
| target_types=[DockerImageTarget], | ||
| ) | ||
|
|
||
|
|
||
| def _make_image_refs(address: Address) -> DockerImageRefs: | ||
| repository = address.target_name | ||
| return DockerImageRefs( | ||
| [ | ||
| ImageRefRegistry( | ||
| registry=None, | ||
| repository=repository, | ||
| tags=( | ||
| ImageRefTag( | ||
| template="latest", | ||
| formatted="latest", | ||
| full_name=f"{repository}:latest", | ||
| uses_local_alias=False, | ||
| ), | ||
| ), | ||
| ) | ||
| ] | ||
| ) | ||
|
|
||
|
|
||
| def create_test_context(rule_runner: RuleRunner, pull_value=None): | ||
| """Helper to create a mock build context and target with specific pull value.""" | ||
| # Create BUILD file with optional pull value | ||
| # Python booleans need to be capitalized (True/False) in BUILD files | ||
| build_content = "docker_image(name='test'" | ||
| if pull_value is not None: | ||
| if isinstance(pull_value, str): | ||
| build_content += f", pull='{pull_value}'" | ||
| else: | ||
| # Convert bool to string with proper capitalization | ||
| build_content += f", pull={str(pull_value)}" | ||
| build_content += ")" | ||
|
|
||
| rule_runner.write_files( | ||
| { | ||
| "test/BUILD": build_content, | ||
| "test/Dockerfile": "FROM alpine:3.16\n", | ||
| } | ||
| ) | ||
|
|
||
| tgt = rule_runner.get_target(Address("test")) | ||
|
|
||
| # Mock build context | ||
| build_context = DockerBuildContext( | ||
| build_args=DockerBuildArgs(), | ||
| digest=EMPTY_DIGEST, | ||
| dockerfile="test/Dockerfile", | ||
| build_env=DockerBuildEnvironment(environment=EnvironmentVars()), | ||
| interpolation_context=InterpolationContext.from_dict( | ||
| { | ||
| "tags": InterpolationValue({}), | ||
| } | ||
| ), | ||
| copy_source_vs_context_source=(("test/Dockerfile", ""),), | ||
| stages=(), | ||
| upstream_image_ids=(), | ||
| ) | ||
|
|
||
| return tgt, build_context | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "policy", | ||
| ["always", "missing", "never", "newer"], | ||
| ) | ||
| def test_podman_pull_string_policies(rule_runner: RuleRunner, policy: str) -> None: | ||
| """Test that Podman accepts all valid string pull policies.""" | ||
| tgt, build_context = create_test_context(rule_runner, pull_value=policy) | ||
|
|
||
| docker_options = create_subsystem( | ||
| DockerOptions, | ||
| registries={}, | ||
| default_repository="{name}", | ||
| default_context_root="", | ||
| build_args=[], | ||
| build_target_stage=None, | ||
| build_hosts=None, | ||
| build_verbose=False, | ||
| build_no_cache=False, | ||
| use_buildx=False, | ||
| env_vars=[], | ||
| ) | ||
|
|
||
| # Use Podman binary | ||
| podman_binary = DockerBinary( | ||
| path="/bin/podman", | ||
| fingerprint="test", | ||
| extra_env={}, | ||
| extra_input_digests=None, | ||
| is_podman=True, | ||
| ) | ||
|
|
||
| address = Address("test") | ||
| image_refs = _make_image_refs(address) | ||
|
|
||
| result: DockerImageBuildProcess = run_rule_with_mocks( | ||
| get_docker_image_build_process, | ||
| rule_args=[ | ||
| DockerPackageFieldSet.create(tgt), | ||
| docker_options, | ||
| podman_binary, | ||
| ], | ||
| mock_calls={ | ||
| "pants.backend.docker.util_rules.docker_build_context.create_docker_build_context": lambda _req: build_context, | ||
| "pants.engine.internals.graph.resolve_target": lambda _: WrappedTarget(tgt), | ||
| "pants.backend.docker.goals.package_image.get_image_refs": lambda _: image_refs, | ||
| }, | ||
| union_membership=UnionMembership.from_rules([]), | ||
| show_warnings=False, | ||
| ) | ||
|
|
||
| # Verify that the correct policy was used | ||
| argv = result.process.argv | ||
| expected_flag = f"--pull={policy}" | ||
| assert expected_flag in argv, f"Expected '{expected_flag}' in {argv}" | ||
|
|
||
|
|
||
| def test_docker_pull_string_raises_error(rule_runner: RuleRunner) -> None: | ||
| """Test that Docker backend raises error when given a string pull policy.""" | ||
| tgt, build_context = create_test_context(rule_runner, pull_value="always") | ||
|
|
||
| docker_options = create_subsystem( | ||
| DockerOptions, | ||
| registries={}, | ||
| default_repository="{name}", | ||
| default_context_root="", | ||
| build_args=[], | ||
| build_target_stage=None, | ||
| build_hosts=None, | ||
| build_verbose=False, | ||
| build_no_cache=False, | ||
| use_buildx=False, | ||
| env_vars=[], | ||
| ) | ||
|
|
||
| # Use Docker binary (not Podman) | ||
| docker_binary = DockerBinary( | ||
| path="/bin/docker", | ||
| fingerprint="test", | ||
| extra_env={}, | ||
| extra_input_digests=None, | ||
| is_podman=False, | ||
| ) | ||
|
|
||
| address = Address("test") | ||
| image_refs = _make_image_refs(address) | ||
|
|
||
| # Should raise InvalidFieldException | ||
| with pytest.raises(InvalidFieldException) as exc_info: | ||
| run_rule_with_mocks( | ||
| get_docker_image_build_process, | ||
| rule_args=[ | ||
| DockerPackageFieldSet.create(tgt), | ||
| docker_options, | ||
| docker_binary, | ||
| ], | ||
| mock_calls={ | ||
| "pants.backend.docker.util_rules.docker_build_context.create_docker_build_context": lambda _req: build_context, | ||
| "pants.engine.internals.graph.resolve_target": lambda _: WrappedTarget(tgt), | ||
| "pants.backend.docker.goals.package_image.get_image_refs": lambda _: image_refs, | ||
| }, | ||
| union_membership=UnionMembership.from_rules([]), | ||
| show_warnings=False, | ||
| ) | ||
|
|
||
| assert "string pull policies are only supported by Podman" in str(exc_info.value) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| import os | ||
| import re | ||
| import warnings | ||
| from abc import ABC, abstractmethod | ||
| from collections.abc import Callable, Iterator | ||
| from dataclasses import dataclass | ||
|
|
@@ -32,6 +33,7 @@ | |
| ListOfDictStringToStringField, | ||
| OptionalSingleSourceField, | ||
| StringField, | ||
| StringOrBoolField, | ||
| StringSequenceField, | ||
| Target, | ||
| Targets, | ||
|
|
@@ -252,7 +254,7 @@ def option_values( | |
|
|
||
| @final | ||
| def options( | ||
| self, value_formatter: OptionValueFormatter, global_build_hosts_options | ||
| self, value_formatter: OptionValueFormatter, global_build_hosts_options, **kwargs | ||
| ) -> Iterator[str]: | ||
| for value in self.option_values( | ||
| value_formatter=value_formatter, global_build_hosts_options=global_build_hosts_options | ||
|
|
@@ -523,19 +525,64 @@ def options(self, *args, **kwargs) -> Iterator[str]: | |
| yield f"{self.docker_build_option}={','.join(list(self.value))}" | ||
|
|
||
|
|
||
| class DockerImageBuildPullOptionField(DockerBuildOptionFieldValueMixin, BoolField): | ||
| class DockerImageBuildPullOptionField(StringOrBoolField): | ||
| alias = "pull" | ||
| default = False | ||
| default = None | ||
| valid_choices = ("always", "missing", "never", "newer") | ||
| help = help_text( | ||
| """ | ||
| If true, then docker will always attempt to pull a newer version of the image. | ||
| Pull policy for the image. | ||
|
|
||
| For Docker: accepts boolean (true to always pull, false to use cached). | ||
| For Podman: accepts boolean or string policy ("always", "missing", "never", "newer"). | ||
| Default: false for Docker, "missing" for Podman. | ||
|
|
||
| NOTE: This option cannot be used on images that build off of "transitive" base images | ||
| referenced by address (i.e. `FROM path/to/your/base/Dockerfile`). | ||
| """ | ||
| ) | ||
| docker_build_option = "--pull" | ||
|
|
||
| def options(self, value_formatter, global_build_hosts_options=None, **kwargs): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function doesn’t feel like idiomatic Pants, but I don’t know exactly why. |
||
| # Determine backend type from DockerBinary (which is resolved based on | ||
| # the [docker].experimental_enable_podman option). When experimental_enable_podman=true, | ||
| # the docker_binary will be 'podman' and is_podman will be True. | ||
| docker_binary = kwargs.get("docker") or kwargs.get("docker_binary") | ||
| is_podman = ( | ||
| getattr(docker_binary, "is_podman", False) if docker_binary is not None else False | ||
| ) | ||
|
|
||
| val = self.value | ||
| if val is None: | ||
| # Use defaults based on backend | ||
| val = "missing" if is_podman else False | ||
|
|
||
| if isinstance(val, str): | ||
| # String policies are only supported by Podman | ||
| if not is_podman: | ||
| raise InvalidFieldException( | ||
| f"The {self.alias!r} field was set to string value {val!r}, " | ||
| f"but string pull policies are only supported by Podman, not Docker. " | ||
| f"Use a boolean value (true/false) for Docker." | ||
| ) | ||
| yield f"{self.docker_build_option}={value_formatter(val)}" | ||
| else: | ||
| # Boolean value | ||
| if is_podman: | ||
| # Convert boolean to Podman policy string | ||
| warnings.warn( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was the reason to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to add a deprecation warning before the support for booleans is dropped for podman in future releases such that the current behavior does not change. |
||
| f"Using boolean values for the 'pull' field with Podman is deprecated. " | ||
| f"Please use string values instead: 'always', 'missing', 'never', or 'newer'. " | ||
| f"Boolean {val} is being converted to 'always' if val else 'missing' policy.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
| policy = "always" if val else "missing" | ||
| yield f"{self.docker_build_option}={policy}" | ||
| else: | ||
| # Docker: emit explicit boolean value with capital first letter | ||
| yield f"{self.docker_build_option}={str(val).capitalize()}" | ||
|
|
||
|
|
||
| class DockerBuildOptionFlagFieldMixin(BoolField, ABC): | ||
| """Inherit this mixin class to provide optional flags (i.e. add `--flag` only when the value is | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1900,6 +1900,39 @@ def compute_value(cls, raw_value: str | None, address: Address) -> str | None: | |
| return value_or_default | ||
|
|
||
|
|
||
| class StringOrBoolField(Field): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I don’t particularly like this target. Is it strictly here to maintain backwards compatibility on the docker_image target?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it is strictly there to maintain backwards compatibility... |
||
| """A field whose value can be either a string or a boolean. | ||
|
|
||
| This is useful for fields that need to accept both boolean flags and string options. | ||
| Subclasses must either set `default: str | bool` or `required = True` so that the value is | ||
| always defined. | ||
|
|
||
| If you expect the string to only be one of several values, set the class property | ||
| `valid_choices`. | ||
| """ | ||
|
|
||
| value: str | bool | None | ||
| default: ClassVar[str | bool | None] = None | ||
| valid_choices: ClassVar[type[Enum] | tuple[str, ...] | None] = None | ||
|
|
||
| @classmethod | ||
| def compute_value( | ||
| cls, raw_value: str | bool | None, address: Address | ||
| ) -> str | bool | None | Any: | ||
| value_or_default = super().compute_value(raw_value, address) | ||
| if value_or_default is not None: | ||
| if not isinstance(value_or_default, (str, bool)): | ||
| raise InvalidFieldTypeException( | ||
| address, cls.alias, raw_value, expected_type="a string or boolean" | ||
| ) | ||
| # Validate string choices if provided | ||
| if isinstance(value_or_default, str) and cls.valid_choices is not None: | ||
| _validate_choices( | ||
| address, cls.alias, [value_or_default], valid_choices=cls.valid_choices | ||
| ) | ||
| return value_or_default | ||
|
|
||
|
|
||
| class SequenceField(Generic[T], Field): | ||
| """A field whose value is a homogeneous sequence. | ||
|
|
||
|
|
||
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.
The writing of this test feels a bit too clever, with all the conditionals. It looks less clean to maintain then maybe a single “file” template that can accept a value maybe.
I had to read it a couple times to actually understand it.