Validate config/manifest documents using Pydantic#74
Conversation
This change moves to using BaseModel instead of a dataclass which has several advantages for validation - Built-in validation - Default values can be set on the class attr - We can add custom validators We also add a new ConfigDocument class that represents the TOML document itself. Additionally, we also change the entities stored as a Set to a List. De-duplication is handled by a field validator.
Test Results135 tests 133 ✅ 5s ⏱️ Results for commit f8a1582. ♻️ This comment has been updated with latest results. |
|
@ianpittwood I fully expect that further validations and test will be required when I start to convert the document objects into the targetbuild/project structure. |
ianpittwood
left a comment
There was a problem hiding this comment.
We should probably consider updating the version in pyproject.toml at least to a new dev version.
Everything looks good so far direction-wise to me! I mostly had questions and some nits on typos and such.
|
|
||
| @field_validator("os", mode="after") | ||
| @classmethod | ||
| def validate_os(cls, _os: List[str]) -> List[str]: |
There was a problem hiding this comment.
For my own understanding, would we be receiving the substring of the Containerfile extension in here? For example (and I know these have been altered) from our existing files ubuntu2204 or centos7
There was a problem hiding this comment.
I haven't gotten to that point yet. I was intending to look at this when I re-worked how the manifests relate to the build plan.
|
|
||
| # Only allow lowercase letters, number, hyphens, underscores, and periods | ||
| # Do not use raw strings here to avoid escaping (e.g. r"{{.+?}}") | ||
| TAG_STR: str = "[a-z0-9-_.]" |
There was a problem hiding this comment.
nit: I prefer globals/constants to always be just after imports or for them to be members of their relevant class. I can also understand why grouping them may be more visible for you though. Perhaps we should consider further breaking down these definitions into additional files and/or modules and utilizing __init__.py to export them for ease of use?
There was a problem hiding this comment.
I originally had these up at the top of the file. I'll move them back up there.
I like your idea of adding more files. I'll think about how to do this, but now I'm thinking separate modules for the config/manifest and making each file much smaller.
There was a problem hiding this comment.
I was thinking maybe something like this:
.
└── models/
├── __init__.py
├── config/
│ ├── __init__.py
│ ├── components.py
│ │ ├── <ConfigRegistry>
│ │ └── <ConfigRepository>
│ ├── document.py
│ │ └── <ConfigDocument>
│ └── model.py
│ └── <Config>
└── manifest/
├── __init__.py
├── components.py
│ ├── <BuildOS>
│ └── <GossConfig>
├── document.py
│ ├── <ManifestGoss>
│ ├── <ManifestBuild>
│ ├── <ManifestTarget>
│ └── <ManifestDocument>
└── model.py
├── <TargetBuild>
└── <Manifest>
But that's just an idea for splitting stuff up in a pattern between the two primary definition types.
I do think this draws attention to a possible issue, however. We may need to make the delineation clearer between classes used for ingesting the Document versus those used for modeling the related Object. I don't think classes like ConfigRegistry or ConfigRepository should necessarily be reused and instead should have ConfigDocumentX classes. I think we want to still clearly separate Document=Validation and Model=Transformation.
There was a problem hiding this comment.
This has been refactored in the latest commits.
I completely agree with keeping the config and manifest document information separate from the usage. In the next PR, I'll be refactoring the Project class and related thingz and we'll see where everything lands.
We will be further refactoring models into smaller files. To keep that change isolated, incorporate the rest of the PR feedback into a separate commit.
445faed to
2d10e9a
Compare
|
@ianpittwood Can you please give this another once-over? Thanks! |
ianpittwood
left a comment
There was a problem hiding this comment.
Just a few issues in os.py mainly and some other suggestions. I think it'll be good once the missed variable renames get fixed :D.
There was a problem hiding this comment.
Same as os.py, perhaps this should be target_goss.py?
There was a problem hiding this comment.
I'm going to leave this one as goss.py since the TargetGoss will come in the next PR.
e8d63de to
f8a1582
Compare
First step in #51