Don't munge types in Docker registry config values. #21649
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjyw
force-pushed
the
no_type_munging_in_docker_registry_options
branch
from
b08c71e
to
2b88f41
Compare
benjyw
commented
Nov 15, 2024
| extra_image_tags=tuple( | ||
| d.get("extra_image_tags", DockerRegistryOptions.extra_image_tags) | ||
| ), | ||
| repository=Parser.to_value_type(d.get("repository"), str, None), |
There was a problem hiding this comment.
This was particularly unnecessary, since all that function does when you pass str as the type is cast...
|
To clarify, I think using the wrong type is a potential sign of larger error, and we shouldn't be attempting to coerce input types. We can validate them, perhaps, but that is a separate thing, and a pretty arbitrary one. |
benjyw
commented
Nov 15, 2024
| @@ -52,13 +51,13 @@ def from_dict(cls, alias: str, d: dict[str, Any]) -> DockerRegistryOptions: | |||
| return cls( | |||
| alias=alias, | |||
| address=d["address"], | |||
There was a problem hiding this comment.
Note that we never validated the type (or presence) of d["address"] for example. I prefer to be uniformly strict than have some arbitrary semi-validation that follows no rhyme or reason.
tdyas
approved these changes
Nov 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.