Add models for a single YAML configuration file#155
Conversation
…ormation (registry/repository info) from config
Add pre-validators to properly interpret a dictionary with "name" and "email" values or a string to defer validation to NameEmail
…rent typing and usability
Upgrade Pydantic to allow utilization of validated data in default_factory functions Make TagPattern objects hashable
Add validation for version dupes Add validation for image dupes Add validation for variant dupes
Test Results375 tests 373 ✅ 1m 15s ⏱️ Results for commit 59be1fe. |
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
I'm really liking the direction of this. Feel free to merge this as you work on pulling in the functionality in the follow-up PR.
My comments generally fall into two groups:
Optional fields
I am getting several warnings for missing parmaeters in model initialization. This occurs when we initialize a pydantic model without all of the fields.
Type assignments
I would like to do all we can to make type/param validation required since others will be using this tool.
pylance is reporting several issues with mismatched types.
I think it's time to start enforcing types as part of the pre-commit hook and in CI.
|
|
||
|
|
||
| class GossOptions(ToolOptions): | ||
| tool: Literal["goss"] = "goss" |
There was a problem hiding this comment.
This complains that tool is not the same type (str) as the parent class.
There was a problem hiding this comment.
I have zero clue how to fix this. I found a Pydantic issue about abstract properties, but attempting to implement a discriminator that way results in errors.
|
|
||
| class Registry(BakeryYAMLModel): | ||
| host: str | ||
| namespace: Annotated[str | None, Field(default=None)] |
There was a problem hiding this comment.
This should be optional
There was a problem hiding this comment.
Python recommends doing the | None syntax over using Optional, but they are functionally equivalent.
https://stackoverflow.com/questions/69440494/python-3-10-optionaltype-or-type-none
| url: HttpUrl | ||
| vendor: Annotated[str, Field(default="Posit Software, PBC")] | ||
| maintainer: Annotated[ | ||
| HashableNameEmail, Field(default=NameEmail(name="Posit Docker Team", email="docker@posit.co")) | ||
| ] | ||
| authors: Annotated[list[HashableNameEmail], Field(default_factory=list)] |
There was a problem hiding this comment.
I believe only url is required, so we should make th other fields optional
There was a problem hiding this comment.
The defaults handle that since all fields should be set or defaulted.
| """Prepend 'https://' to the URL if it does not already start with it""" | ||
| if isinstance(value, str): | ||
| if not value.startswith("https://"): | ||
| value = f"https://{value}" |
There was a problem hiding this comment.
Type "str" is not assignable to declared type "AnyUrl"
"str" is not assignable to "Url"PylancereportAssignmentType
There was a problem hiding this comment.
I changed the type hints to value: Any and -> Any which should satisfy it. Since it's a before validator, we should be able to take any value. In this case, I wanted to defer the conversion of the string to HttpUrl to Pydantic's own validator. If there's a better way of doing that though, let me know.
2 participants
BakeryConfig- Manager for theBakeryConfigDocument. This class will handle nearly all interactions from the CLI and other code in the future.BakeryConfigDocument- Top-level model for the configuration file with fields for the three main sections:repository,registries, andimages. It also registers the base path for the project (the directory in which the config resides). Previous validators should be implemented. I also added a capability for a lot of these objects to register themselves as aparentin each child to make it easier to traverse back and forth across the config.Repository- Basically a direct rip of the existingRepositorymodel. The only major difference is that contact info is now stored as aHashableNameEmailtype, a child of Pydantic'sNameEmailwith a hash function for deduplication.urlis now stored as anHttpUrltype too to enforce it being a URL. I did add a pre-validator to prependhttps://if it's not that since it causes errors otherwise.Registry- Direct rip of existingRegistrymodel.Image- Represents anImagein the config and itsvariants(rename of types such asminandstd) andversions. Registries can also be added here to the global registries or overridden entirely usingoverrideRegistries. Image-widetagPatternscan also be set here.ImageVariant- Represents a variant of an image like Standard (std) or Minimal (min). Standard is no longer the de factor primary variant and a primary should instead be specified by settingprimary. The extension searched for when searching for Containerfiles (exContainerfile.min.ubuntu2204) can now be set usingextension, but it will automatically be guessed at if not provided using the name. ThetagDisplayNamecan also be overridden for what is provided to tag pattern rendering. Variants can also add tag patterns, but they cannot currently modify registries.Imageuses "Standard" and "Minimal" variants by default where "Standard" is set as primary. They also using the shortened namesstdandminrespectively for the extension and tag display names respectively.ImageVersion- Represents a version of the image. It now allows overriding the directory name via thesubpathto make it so we can shorten Workbench versions to their core versions without metadata on directory names if we want. The verison still takes thelatestflag and a list ofos. Versions can set their own registries and override registries, but cannot set their own tags.ImageVersionOS- Represents an operating system used with a version. The OS config still retains theprimaryflag for tagging purposes. It also addsextensionandtagDisplayNamefields to override the behavior for Containerfile matching and how tags are abbreviated.TagPattern- Represents a Jinja2 string pattern to render into a tag suffix. There's a default generator for these that should cover existing cases. There's an enum that represents filters for the tags. There's no complex logic currently implemented and they just useANDfor each filter applied.