release: stop putting ostree commits into release metadata #4319
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the handling of ostree commit information from the release metadata generation and processing, reflecting the project's move to OCI-based updates. The changes correctly remove the logic for propagating and comparing ostree commits in mantle/cmd/plume/release.go and stop adding the commit information to release.json in src/cmd-generate-release-meta. The modifications are clean and align with the stated goal. I have identified a critical pre-existing issue in a modified code block that could lead to a panic due to a nil pointer dereference and have provided a fix.
| for arch, vals := range rel.Architectures { | ||
| commits = append(commits, release.IndexReleaseCommit{ | ||
| Architecture: arch, | ||
| Checksum: vals.Commit, | ||
| }) | ||
| pullspecs = append(pullspecs, release.IndexReleaseOciImage{ | ||
| Architecture: arch, | ||
| ContainerImage: *vals.OciImage, | ||
| }) | ||
| } |
There was a problem hiding this comment.
There is a potential nil pointer dereference here. vals.OciImage can be nil if the release.json for a given architecture doesn't contain an oci-image field. The python script that generates release.json (src/cmd-generate-release-meta) only includes oci-image if base-oscontainer is present in its input, with a comment noting this may not always be the case (e.g. for OKD).
Dereferencing vals.OciImage without a nil check could cause a panic. You should add a check to ensure vals.OciImage is not nil before using it.
for arch, vals := range rel.Architectures {
if vals.OciImage == nil {
continue
}
pullspecs = append(pullspecs, release.IndexReleaseOciImage{
Architecture: arch,
ContainerImage: *vals.OciImage,
})
}
|
There is part of me that would like to continue to serve up entries in the graph until all the F42 builds have been done. This removes that ability, correct? Our change request is really for F43: coreos/fedora-coreos-tracker#1895 (comment) |
Yes, and yeah agree. Don't know. I guess we can add a knob for it. Or maybe simplest is to tie it to the buildah path work, which is also tied to f43. So then we could just check here if it has |
sounds fair to me |
jlebon
force-pushed
the
pr/drop-ostree
branch
from
ee0373f to
ded913d
Compare
Starting in f43, we won't be publishing content to the OSTree repo anymore, so there's no point in having any commit hash in the release metadata. On builds that have `oci-imported: true`, let's stop adding the `commit` key in the release metadata in `cosa generate-release-metadata`. That'll naturally bind it to f43. In plume, handle the case where there is no commit string.
Prep for next patch.
We want to make sure that all of developers, CI and pipelines default to the same buildah vs legacy path. Let's make the config repo itself drive this. Support a new `.metadata.build_with_buildah` key in `manifest.yaml` which, if set to true, will make `cosa build` automatically use the buildah path. Note the `COSA_BUILD_WITH_BUILDAH` env var is still supported and can be used to override the default either way (i.e., set to `0`, it will force the legacy path).
Adapt to coreos/fedora-coreos-config#3724 which changed the path of the source content slightly.
jlebon
force-pushed
the
pr/drop-ostree
branch
from
ded913d to
a1b18da
Compare
|
Cincinnati change to handle this : coreos/fedora-coreos-cincinnati#107 |
We've fully cut over to OCI now for updates so we can stop having ostree commit info in there. It'll make even less sense with the buildah path where the commit hash is synthetic.
In
cosa generate-release-metadata, stop propagating the commit. In plume, stop propagating the commit to the release index.