rpm_ostree/cli_status: Support com.coreos.stream OCI config label for stream extraction
#1321
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 updates the stream extraction logic to support the com.coreos.stream OCI label, with a fallback to the older fedora-coreos.stream annotation. The changes look good and correctly implement the desired logic. I've suggested a small refactoring to the fedora_coreos_stream_from_deployment function to improve its clarity and error reporting by removing a redundant check and making the final error message more comprehensive.
src/rpm_ostree/cli_status.rs
Outdated
| if deploy.base_commit_meta.stream.is_none() | ||
| && deploy.base_commit_meta.oci_image_configuration.is_none() | ||
| && deploy.base_commit_meta.oci_manifest.is_none() | ||
| { | ||
| bail!("Cannot deserialize ostree base image manifest"); | ||
| } | ||
|
|
||
| if let Some(stream) = deploy.base_commit_meta.stream.as_ref() { | ||
| if !stream.is_empty() { | ||
| return Ok(stream.clone()); | ||
| } | ||
| } | ||
|
|
||
| // Check for `com.coreos.stream` label in OCI ImageConfiguration | ||
| if let Some(oci_image_configuration) = deploy.base_commit_meta.oci_image_configuration.as_ref() | ||
| { | ||
| let image_configuration: ostree_ext::oci_spec::image::ImageConfiguration = | ||
| serde_json::from_str(oci_image_configuration.as_str())?; | ||
| if let Some(stream) = image_configuration.config().as_ref().and_then(|cfg| { | ||
| cfg.labels() | ||
| .as_ref() | ||
| .and_then(|labels| labels.get("com.coreos.stream")) | ||
| }) { | ||
| if !stream.is_empty() { | ||
| return Ok(stream.clone()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback to `fedora-coreos.stream` label in OCI ImageManifest | ||
| if let Some(oci_manifest) = deploy.base_commit_meta.oci_manifest.as_ref() { | ||
| let manifest: ostree_ext::oci_spec::image::ImageManifest = | ||
| serde_json::from_str(oci_manifest.as_str())?; | ||
| if let Some(stream) = manifest | ||
| .annotations() | ||
| .as_ref() | ||
| .and_then(|a| a.get("fedora-coreos.stream")) | ||
| { | ||
| if !stream.is_empty() { | ||
| return Ok(stream.clone()); | ||
| } | ||
| } | ||
| (None, None) => bail!("Cannot deserialize ostree base image manifest"), | ||
| }; | ||
| ensure!(!stream.is_empty(), "empty stream value"); | ||
| Ok(stream) | ||
| } | ||
|
|
||
| return Err(anyhow!( | ||
| "Missing `com.coreos.stream` label in the base image configuration, | ||
| or `fedora-coreos.stream` annotation in the base image manifest" | ||
| )); |
There was a problem hiding this comment.
This function can be simplified for better readability and more helpful error reporting.
- The initial check for all sources being
Noneis redundant. If all optional fields areNone, the function will correctly fall through to the final error. Removing this check simplifies the code. - The final error message is incomplete. It should mention all three possible sources of the stream information (
base_commit_meta.stream,oci_image_configuration, andoci_manifest) to be more helpful for debugging.
Here is a suggested refactoring that addresses these points:
if let Some(stream) = deploy.base_commit_meta.stream.as_ref() {
if !stream.is_empty() {
return Ok(stream.clone());
}
}
// Check for `com.coreos.stream` label in OCI ImageConfiguration
if let Some(oci_image_configuration) = deploy.base_commit_meta.oci_image_configuration.as_ref()
{
let image_configuration: ostree_ext::oci_spec::image::ImageConfiguration =
serde_json::from_str(oci_image_configuration.as_str())?;
if let Some(stream) = image_configuration.config().as_ref().and_then(|cfg| {
cfg.labels()
.as_ref()
.and_then(|labels| labels.get("com.coreos.stream"))
}) {
if !stream.is_empty() {
return Ok(stream.clone());
}
}
}
// Fallback to `fedora-coreos.stream` label in OCI ImageManifest
if let Some(oci_manifest) = deploy.base_commit_meta.oci_manifest.as_ref() {
let manifest: ostree_ext::oci_spec::image::ImageManifest =
serde_json::from_str(oci_manifest.as_str())?;
if let Some(stream) = manifest
.annotations()
.as_ref()
.and_then(|a| a.get("fedora-coreos.stream"))
{
if !stream.is_empty() {
return Ok(stream.clone());
}
}
}
Err(anyhow!(
"Missing or empty stream from all sources: `fedora-coreos.stream` in commit meta, `com.coreos.stream` in OCI config, or `fedora-coreos.stream` in OCI manifest"
))
src/rpm_ostree/cli_status.rs
Outdated
| #[derive(Clone, Debug, Deserialize)] | ||
| struct BaseCommitMeta { | ||
| #[serde(rename = "fedora-coreos.stream")] | ||
| stream: Option<String>, |
There was a problem hiding this comment.
This was the old old fallback. We should probably drop it now and stop considering it in the code below.
There was a problem hiding this comment.
Removed the old fallback code but had to copy the ostree.container.image-config from rpm-ostree-oci-status.json fixture to the rpm-ostree-status.json fixture for tests to pass correctly
There was a problem hiding this comment.
As I wrote in my review below this is effectively breaking support for non OCI deployements I think.
I am OK to do this but this should be done in the other relevant places too, and probably in a separate release
src/rpm_ostree/cli_status.rs
Outdated
| if let Some(stream) = deploy.base_commit_meta.stream.as_ref() { | ||
| if !stream.is_empty() { | ||
| return Ok(stream.clone()); | ||
| } | ||
| } |
Roshan-R
force-pushed
the
fix/replace-stream-annotation-with-label
branch
4 times, most recently
from
cfd03d6 to
9744cd5
Compare
|
Thanks - the code is looking good. I would like to test it myself if I can get a chance. Let's also delete The reason we had both is we were trying to test both cases (from the old fallback). Or actually I guess we should pivot and still have two:
|
Roshan-R
force-pushed
the
fix/replace-stream-annotation-with-label
branch
from
9744cd5 to
bc47465
Compare
|
The tests have been updated |
jbtrystram
left a comment
jbtrystram
left a comment
There was a problem hiding this comment.
This looks generally sane to me, i still want to test it manually.
One thing this does is breaking support for encapsulated ostree deployments built with COSA.
This is probably OK since we won't ship newer Zincati in old-style images but that's something we want to think about, and if we drop support for those we should do it accross the whole codebase I think.
|
steps to test the PR. I have uploaded the modified status.json with the |
|
Nice, thanks Roshan ! |
Roshan-R
force-pushed
the
fix/replace-stream-annotation-with-label
branch
from
bc47465 to
8476170
Compare
| } | ||
|
|
||
| // Fallback to `fedora-coreos.stream` label in OCI ImageManifest | ||
| if let Some(oci_manifest) = deploy.base_commit_meta.oci_manifest.as_ref() { |
There was a problem hiding this comment.
It's weird that we have to get the labels through ImageConfig and the annotation through ImageManifest
Looking deeper I find that annotations and labels are basically the same thing (except annotations can be added to multiples files in an image), annotation being the OCI spec way to do it and labels being from Docker.
oh well, now i wonder if we should have done an annotation :)
Interesting read : https://adrianmouat.com/posts/annotations-and-labels-in-container-images/
Exactly. We'll never ship a new release of zincati with the old style (non container based) deployments. The only reason to keep it I can think of is if someone other than us were using Zincati and I don't know of anyone doing that.
SGTM. I think now would be the best time to do this. Do you? |
…or stream extraction Update the stream extraction logic to support the `com.coreos.stream` label in the OCI image configuration metadata. Previously, the stream was determined using the `fedora-coreos.stream` annotation. While that annotation is still supported as a fallback, the preferred method is now to use the new `com.coreos.stream` label. This change aligns with the new Konflux-built images, which include only this label and no longer provide the older annotation. See: coreos/fedora-coreos-config#3867
Roshan-R
force-pushed
the
fix/replace-stream-annotation-with-label
branch
from
8476170 to
a142c98
Compare
3 participants
Update the stream extraction logic to support the
com.coreos.streamlabel in the OCI image configuration metadata.Previously, the stream was determined using the
fedora-coreos.streamannotation. While that annotation is still supported as a fallback, the preferred method is now to use the newcom.coreos.streamlabel. This change aligns with the new Konflux-built images, which include only this label and no longer provide the older annotation.See: coreos/fedora-coreos-config#3867