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

status: Add version into status --json #3251

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

I wanted to add some code into openshift/machine-config-operator
which checked the rpm-ostree version. We can do this today with
rpm-ostree --version or rpm -q rpm-ostree, but the MCO today
already reads rpm-ostree status --json.

It's much more natural to parse JSON for this as opposed to forking
the binary again or querying rpm. (The latter in particular would
require splitting on the upstream version vs release tag etc.)

I wanted to add some code into openshift/machine-config-operator
which checked the rpm-ostree version.  We can do this today with
`rpm-ostree --version` or `rpm -q rpm-ostree`, but the MCO today
already reads `rpm-ostree status --json`.

It's much more natural to parse JSON for this as opposed to forking
the binary again or querying rpm.  (The latter in particular would
require splitting on the upstream version vs release tag etc.)
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, just forking rpm-ostree --version again doesn't seem like a big deal. It's in YAML format too, so it's ready to parse and has even more versioning info! :)

Having it in status --json feels a bit odd IMO, but not strongly against either.

@cgwalters
Copy link
Member Author

Hmm, just forking rpm-ostree --version again doesn't seem like a big deal. It's in YAML format too, so it's ready to parse and has even more versioning info! :)

I wrote a whole rationale in the commit message about this - specifically JSON vs YAML, and the fact that we are already parsing the status output.

Probably though what we really want here is official Go bindings #2389

@jlebon
Copy link
Member

jlebon commented Dec 6, 2021

Hmm, just forking rpm-ostree --version again doesn't seem like a big deal. It's in YAML format too, so it's ready to parse and has even more versioning info! :)

I wrote a whole rationale in the commit message about this - specifically JSON vs YAML, and the fact that we are already parsing the status output.

Right, I get that it's the most convenient thing to do for the MCO. But we should still weigh the different options. For example, I see you've already got something using rpm-ostree --version in openshift/machine-config-operator#2859. Most of that code is parsing the string itself, which would be the same regardless. But we could make that easier if we add a e.g. VersionYear and VersionRelease already integer typed in the --version output.

Probably though what we really want here is official Go bindings #2389

👍

@cgwalters
Copy link
Member Author

For example, I see you've already got something using rpm-ostree --version in openshift/machine-config-operator#2859. Most of that code is parsing the string itself, which would be the same regardless

Yes, but I only wrote all that code because we didn't have the version in status 😄

cgwalters added a commit to cgwalters/machine-config-operator that referenced this pull request Dec 6, 2021
This is *mainly* to validate that
openshift/release#24225
worked.

But, this code may be useful as a sanity check going forward.

See also coreos/rpm-ostree#3251

(I also may try to expose e.g. `ex-container` as a feature flag
 that we can query instead of version-parsing)
@jlebon
Copy link
Member

jlebon commented Dec 8, 2021

For example, I see you've already got something using rpm-ostree --version in openshift/machine-config-operator#2859. Most of that code is parsing the string itself, which would be the same regardless

Yes, but I only wrote all that code because we didn't have the version in status 😄

What I mean is wouldn't most of that code be the same though? Since we'd still have to parse a version string.

(Again to clarify, I'm OK with this. But I think there's an opportunity to make that MCO code way simpler if we just extend rpm-ostree --version. Or we could throw the new integer fields into status --json too, but that's even more awkward.)

@cgwalters
Copy link
Member Author

Eh, we can revisit this later.

@cgwalters cgwalters closed this Jan 24, 2022
cheesesashimi pushed a commit to cheesesashimi/machine-config-operator that referenced this pull request Feb 22, 2022
This is *mainly* to validate that
openshift/release#24225
worked.

But, this code may be useful as a sanity check going forward.

See also coreos/rpm-ostree#3251

(I also may try to expose e.g. `ex-container` as a feature flag
 that we can query instead of version-parsing)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants