Conversation
Test Results818 tests 818 ✅ 9m 37s ⏱️ Results for commit d0f51b3. ♻️ This comment has been updated with latest results. |
bschwedler
left a comment
bschwedler
left a comment
There was a problem hiding this comment.
Changes here make sense to me.
Just a few comments/questions on how merging of the amd/arm images will be performed. I am currently assuing that is a follow-up PR.
| target.metadata_file = MetadataFile(target_uid=target.uid, filepath=metadata_filepath) | ||
|
|
||
| merged_metadata = config._merge_sequential_build_metadata_files() | ||
| with open(CONFIG_TESTDATA_DIR / "build_metadata" / "expected.json", "r") as f: |
There was a problem hiding this comment.
It looks like we just have amd builds here. Am I correct in assuming that the arm builds will be added in a follow-up PR.
There was a problem hiding this comment.
The distinction will not matter for this test or for multiplatform support. If a platform build is performed in isolation, it will produce one metadata file with unique targets. If a multiplatform build is performed with emulation, it will still only produce one metadata file with unique targets. The merge functionality only exists because sequential builds produce a metadata file per image target and I think it's easier for us to produce one file with every image target, which is what bake would produce if my PR goes through.
| "IMAGE_OS_VERSION=22.04", | ||
| "--init", | ||
| basic_standard_image_target.tags[0], | ||
| basic_standard_image_target.ref, |
There was a problem hiding this comment.
I really like this. Much more deterministic!
| @computed_field | ||
| @property | ||
| def ref(self) -> str: | ||
| """Returns a reference to the image, preferring a build metadata digest if available.""" |
There was a problem hiding this comment.
What scenarios will we not have the build metadata? Is that currently only if using bake?
There was a problem hiding this comment.
We will practically never have build metadata right now. This is pretty much only to support dgoss calls when --metadata-file is passed in. In the future, it would be nice if dgoss and other tool calls could read build metadata that we cached for the application in some way, but that won't be the case for a while.
| merged_metadata: dict[str, dict[str, Any]] = {} | ||
| for target in self.targets: | ||
| if target.metadata_file is not None: | ||
| merged_metadata[target.uid] = target.metadata_file.metadata.model_dump(exclude_none=True, by_alias=True) |
There was a problem hiding this comment.
It looks like we are just accumulating these by id.
Will the platform merge be performed with the bakery ci merge command?
There was a problem hiding this comment.
bakery ci merge will be the only command where we expect multiple builds of the same image target. My original implementation of it was largely logic outside of the rest of the modeling and I expect it to stay that way.
ianpittwood
force-pushed
the
typer-usage-improvements
branch
3 times, most recently
from
8cc18e7 to
35e2423
Compare
ianpittwood
force-pushed
the
metadata-file-outputs
branch
from
a32b953 to
e22f9bb
Compare
ianpittwood
force-pushed
the
typer-usage-improvements
branch
from
35e2423 to
727679e
Compare
Create settings.py file to manage global paths and settings
…ta files on sequential builds
Add test for image target build with metadata file
ianpittwood
force-pushed
the
metadata-file-outputs
branch
from
e22f9bb to
d0f51b3
Compare
2 participants
Related to #265
--metadata-fileoption tobakery buildto write all build metadata to a JSON file.BakeryConfigto merge data into a single file output.run dgoss.