Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parse OCI deployment status into ImageManifest #5208

Closed
wants to merge 1 commit into from

Conversation

jbtrystram
Copy link
Collaborator

When the deployment is an OCI image, the base_commit_meta field contains serialized JSON.

So the values in this map can either be:
- Regular OSTree case: a JSON object
- OCI case : a string containing a serialized JSON object

In the OCI case, simply deserialize the strings before passing them to serde, when necessary.

See #5196

Copy link

openshift-ci bot commented Jan 6, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cgwalters
Copy link
Member

A lot of discussion in #5196

@cgwalters
Copy link
Member

The json-in-json we have now is obviously silly and unergonomic, and I am not opposed to trying to just change it. However, it is possible that something is parsing it as is today and there's some risk of breakage from that.

Per discussion in the issue, my inclination is basically to try to fix this a different way for now - ideally one that ends up using bootc and leave rpm-ostree as is.

@jlebon
Copy link
Member

jlebon commented Jan 7, 2025

Per discussion in the issue, my inclination is basically to try to fix this a different way for now - ideally one that ends up using bootc and leave rpm-ostree as is.

To be able to always query that info from bootc, I think it also needs to support fishing it out of the OSTree commit even if there are layered packages.

Anyway, feels weird to me to even have these keys in the --json output. We should probably filter it out like we do the rpmdb. OTOH, that would also technically be an API break at this point.

I guess forking ostree container image metadata is OK, though Zincati could also just re-deserialized the string itself since that's already in the rpm-ostree status output it captures. It's ugly but that'll also get cleaned up once we move to bootc.

@jbtrystram
Copy link
Collaborator Author

The json-in-json we have now is obviously silly and unergonomic, and I am not opposed to trying to just change it. However, it is possible that something is parsing it as is today and there's some risk of breakage from that.

I guess forking ostree container image metadata is OK, though Zincati could also just re-deserialized the string itself since that's already in the rpm-ostree status output it captures. It's ugly but that'll also get cleaned up once we move to bootc.

Just as clarification : I opened this PR here because I noticed this crate shares a lot of code with https://github.com/coreos/zincati/blob/main/src/rpm_ostree/cli_status.rs and thought the better approach was to fix it here and reuse that crate in Zincati.
I can however try to do the same in zincati, if we don´t want to change this crate.

@cgwalters
Copy link
Member

coreos/zincati#491 was work to have zincati actually use this code

@cgwalters
Copy link
Member

Closing in favor of coreos/zincati#1241 which is I think what makes sense for now (but still open to adding somewhat similar changes here if desired, though it's not clear to me that it's worth it).

@cgwalters cgwalters closed this Jan 23, 2025
@jbtrystram jbtrystram changed the title [WIP] parse OCI deployment status parse OCI deployment status into ImageManifest Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants