Merged
Conversation
Contributor
bschwedler
commented
Jan 27, 2025
bschwedler
commented
- Use files to store jinja2 templates
- Refactor project.py into models/project subdirectory
- Add basic Image model and matrix tests
- Add load method for ImageVariant
- Add labels and tags to the ImageVariant model
- Add new load method for reading manifest/config
- WIP: Move to using new Config.load method
- Move hard-coded manifest fixtures to separate file
- Add BakePlan object and initial basic test cases
- WIP: Move project tests to new Manifest/Config/Image objects
- Render the bake plan using the BakePlan class
- Use model_dump_json to convert BakePlan to json
- Set all BaseModels to frozen=True after initialized
- Get basic manifest tests working
- Remove older _load method and swich to the Pydantic way
- Skip goss command tests until the are refactored
- Create Images class, used to reference and filter images
- Move creating goss commands to updated Manifest objects
- Create ImageMetadata class for intermediate data
- Patch is_file and is_dir at the test file level
- Move full tag and label generation to ImageVariant
- Remove remaining old tests
Instead of defining the templates as a multi-line string, we instead move the templates to files. The templating module reads these files into the same variable names as before.
These tests check that the number of image versions and variants are what we would expect by combining imave verions, build oses, and targets
This change also adds the functionality to search for the Containerfile based on the OS and target.
This sets image labels at various levels throughout the parent Image and ImageVersion classes. We intend that these labels will be assembled in the bake plan, and the image tag prefixes will prefixed there. Tags are set at the ImageVariant level since tags will be specific to the OS and target.
Rename the current load method to `_load` This allows us to iteratively move test cases over to the new load method while allowing the current tests to continue to pass
This also moves the filtering of images into bake.py
We still need to move the TargetBuild tests to the appropriate test section, removing duplicates
This was required since we need to be able to filter the images in multiple ways, in different parts of the code. The Images class is a dictionary with a couple of extra methods
This is used so that we can pass fewer values around when constructing the Image and ImageVariant objects.
Prior to this change, each test that required us to patch either - pathlib.Path.is_file - pathlib.Path.is_dir would require context handlers around each call. We switch to a file-based fixture which is auto-used
This functionality was originally included in the BakePlan.create method, but we also need it to render the goss information.
Test Results137 tests 134 ✅ 47s ⏱️ Results for commit 17f89aa. ♻️ This comment has been updated with latest results. |
bschwedler
force-pushed
the
toml-schema-translation
branch
from
5e77d0b to
9ce30b9
Compare
ianpittwood
requested changes
Jan 28, 2025
Contributor
ianpittwood
left a comment
ianpittwood
left a comment
There was a problem hiding this comment.
This first pass was mostly trying out commands and making sure things worked. I got everything to work except the render command. In this draft PR I made it works for re-rendering existing versions, but truncates the OS for some reason. It does not work for me on new versions. Everything was tested against the PPM PR to get things updated with no additional changes.
Let's fix up the top-level commands fully and I'll continue additional review for stuff I've missed.
Comment on lines
+35
to
+36
| document = cls.read(filepath) | ||
| model = ManifestDocument(**document.unwrap()) |
Contributor
There was a problem hiding this comment.
This part is confusing to me at the moment. Why are we keeping two copies of the same data? Which one should be modified? What's the dump strategy for writing back changes?
Contributor
Author
There was a problem hiding this comment.
I wanted to achieve 2 things:
- Store the raw
TOMLDocumentobject, since that includes more information like comments - Store the validated data as a pydantic model, which won't change after being read.
As far as writing out changes, I think that it will probably start with a pydantic model, then get pulled back into the TOMLDocument to write.
bschwedler
commented
Jan 29, 2025
Contributor
Author
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
Thanks for the review. I'm going thorugh and implementing the easy fixes, then I'll move over to the new methods, hopefully adding some functional tests along the way.
Comment on lines
+35
to
+36
| document = cls.read(filepath) | ||
| model = ManifestDocument(**document.unwrap()) |
Contributor
Author
There was a problem hiding this comment.
I wanted to achieve 2 things:
- Store the raw
TOMLDocumentobject, since that includes more information like comments - Store the validated data as a pydantic model, which won't change after being read.
As far as writing out changes, I think that it will probably start with a pydantic model, then get pulled back into the TOMLDocument to write.
Add --cov-report=term-missing to `just test` The GitHub Actions workflow uses poetry directly and does not invoke the justfile.
This allows us to load common files for all tests, enabling re-use of common step definitions.
This was referenced Jan 30, 2025
This also adds the git commit to the revision label in the bake plan We still need to address the creation of new projects, images, and versions
This refactor was done so that we could more easily scope the .new() method to create a project, image, or version to each file.
Default image templates are created in the templating module
This will prevent unintentional changes to the list of OSes as they are used elsewhere.
The implements the following changes - Small format changes to CLI output - Move the template rendering into the templating module - Search for BuildOS by pretty or condensed name We still need to implement updating the manifest file for new versions
bschwedler
force-pushed
the
toml-schema-translation
branch
9 times, most recently
from
696d391 to
2e6c75e
Compare
- Add a `slow` pytest marker and skip by default in justfile - Add a reset() method to BakeryCommand to allow us to invoke the command multiple times using the same test fixtures - Install goss and dgoss for GitHub Actions
bschwedler
force-pushed
the
toml-schema-translation
branch
from
2e6c75e to
e494b29
Compare
This also sorts the image versions in descending order
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.