Skip to content

Commit

Permalink
Don't munge types in Docker registry config values. (#21649)
Browse files Browse the repository at this point in the history
We document a schema for the registries dict, including
specifying that certain fields should be bool-valued.

I see no need to coerce those values into bools if they
were actually strings, especially since we don't document
that we do that (AFAICT). 

We should require users to adhere to the published schema.
If this "breaks" users, well, they were accidentally relying on
unadvertised leniency, and the fix is extremely easy. 

Note that this coercion was very ad hoc. We don't apply it
uniformly across all fields in the docker registry config, 
and we certainly don't do this in other dict-valued options
across the repo.

This gets rid of a spurious dependency of the Docker backend
on internal code of the legacy options parser.
  • Loading branch information
benjyw authored Nov 15, 2024
1 parent 2347436 commit e3d7222
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 16 deletions.
10 changes: 10 additions & 0 deletions docs/notes/2.25.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ The "legacy" options system is removed in this release. All options parsing is n

### Backends

#### Docker

Strict adherence to the [schema of Docker registry configuration](https://www.pantsbuild.org/2.25/reference/subsystems/docker#registries) is now required.
Previously we did ad-hoc coercion of some field values, so that, e.g., you could provide a "true"/"false" string as a boolean value. Now we require actual booleans.

#### Helm

Strict adherence to the [schema of Helm OCI registry configuration](https://www.pantsbuild.org/2.25/reference/subsystems/helm#registries) is now required.
Previously we did ad-hoc coercion of some field values, so that, e.g., you could provide a "true"/"false" string as a boolean value. Now we require actual booleans.

### Plugin API changes

## Full Changelog
Expand Down
9 changes: 4 additions & 5 deletions src/python/pants/backend/docker/registries.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from dataclasses import dataclass
from typing import Any, Iterator

from pants.option.parser import Parser
from pants.util.frozendict import FrozenDict
from pants.util.strutil import softwrap

Expand Down Expand Up @@ -52,13 +51,13 @@ def from_dict(cls, alias: str, d: dict[str, Any]) -> DockerRegistryOptions:
return cls(
alias=alias,
address=d["address"],
default=Parser.ensure_bool(d.get("default", alias == "default")),
skip_push=Parser.ensure_bool(d.get("skip_push", DockerRegistryOptions.skip_push)),
default=d.get("default", alias == "default"),
skip_push=d.get("skip_push", DockerRegistryOptions.skip_push),
extra_image_tags=tuple(
d.get("extra_image_tags", DockerRegistryOptions.extra_image_tags)
),
repository=Parser.to_value_type(d.get("repository"), str, None),
use_local_alias=Parser.ensure_bool(d.get("use_local_alias", False)),
repository=d.get("repository"),
use_local_alias=d.get("use_local_alias", False),
)

def register(self, registries: dict[str, DockerRegistryOptions]) -> None:
Expand Down
8 changes: 4 additions & 4 deletions src/python/pants/backend/docker/registries_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ def test_docker_registries() -> None:
# Test defaults.
registries = DockerRegistries.from_dict(
{
"reg1": {"address": "myregistry1domain:port", "default": "false"},
"reg2": {"address": "myregistry2domain:port", "default": "true"},
"reg3": {"address": "myregistry3domain:port", "default": "true"},
"reg1": {"address": "myregistry1domain:port", "default": False},
"reg2": {"address": "myregistry2domain:port", "default": True},
"reg3": {"address": "myregistry3domain:port", "default": True},
}
)

Expand All @@ -65,7 +65,7 @@ def test_skip_push() -> None:
{
"reg1": {"address": "registry1"},
"reg2": {"address": "registry2", "skip_push": True},
"reg3": {"address": "registry3", "skip_push": "false"},
"reg3": {"address": "registry3", "skip_push": False},
}
)

Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/backend/helm/goals/publish_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def test_helm_push_no_charts_when_registries_are_not_set(rule_runner: RuleRunner
def test_helm_skip_push(rule_runner: RuleRunner) -> None:
_declare_targets(rule_runner)

registries = {"internal": {"address": "oci://www.example.com/internal", "default": "true"}}
registries = {"internal": {"address": "oci://www.example.com/internal", "default": True}}
chart_metadata = HelmChartMetadata("foo-chart", "0.1.0")
result, _ = _run_publish(
rule_runner, Address("src/skip-push"), chart_metadata, registries=registries
Expand All @@ -143,7 +143,7 @@ def test_helm_skip_push(rule_runner: RuleRunner) -> None:
def test_helm_push_use_default_registries(rule_runner: RuleRunner) -> None:
_declare_targets(rule_runner)

registries = {"internal": {"address": "oci://www.example.com", "default": "true"}}
registries = {"internal": {"address": "oci://www.example.com", "default": True}}
chart_metadata = HelmChartMetadata("missing-registries", "0.2.0")
result, helm = _run_publish(
rule_runner, Address("src/missing-registries"), chart_metadata, registries=registries
Expand Down
3 changes: 1 addition & 2 deletions src/python/pants/backend/helm/resolve/remotes.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from dataclasses import dataclass
from typing import Any, Iterator, cast

from pants.option.parser import Parser
from pants.util.frozendict import FrozenDict
from pants.util.memo import memoized_method

Expand Down Expand Up @@ -39,7 +38,7 @@ def from_dict(cls, alias: str, d: dict[str, Any]) -> HelmRegistry:
return cls(
alias=alias,
address=cast(str, d["address"]).rstrip("/"),
default=Parser.ensure_bool(d.get("default", alias == "default")),
default=d.get("default", alias == "default"),
)

def __post_init__(self) -> None:
Expand Down
6 changes: 3 additions & 3 deletions src/python/pants/backend/helm/resolve/remotes_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ def test_helm_remotes() -> None:
remotes = HelmRemotes.from_dict(
{
"default": {"address": "oci://www.example.com/default"},
"reg1": {"address": "oci://www.example.com/charts1", "default": "false"},
"reg2": {"address": "oci://www.example.com/charts2", "default": "true"},
"reg3": {"address": "oci://www.example.com/charts3", "default": "true"},
"reg1": {"address": "oci://www.example.com/charts1", "default": False},
"reg2": {"address": "oci://www.example.com/charts2", "default": True},
"reg3": {"address": "oci://www.example.com/charts3", "default": True},
},
)

Expand Down

0 comments on commit e3d7222

Please sign in to comment.