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

rpm-ostree: Use upstream Rust bindings #491

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cgwalters
Copy link
Member

rpm-ostree: Use upstream Rust bindings

Depends: coreos/rpm-ostree#2636

Use the upstream client bindings for the status data. Note
that some functions switched to returning borrowed &String
because there's no need to clone unnecessarily.

An interesting note: In the upstream API, the commit
metadata is exposed as a generic HashMap<> because
that's how it's used by both ostree and rpm-ostree. It'd
be a layering violation for us to hardcode coreos-assembler.basearch
in the rpm-ostree git for example, not to mention fedora-coreos.stream.
So those constants stay here in zincati.

I dropped the _json terminology from various functions
because that's weird - we parsed the data from JSON, but
that's not really very relevant except as an implementation
detail. It's just a Deployment, not a DeploymentJSON.

Also I dropped for now the optimization of using --booted;
it's not a huge amount of data. If we care we can re-add that
later. Note it makes caching more complex because then we
need to carefully check the booted state too.

@cgwalters
Copy link
Member Author

Draft until the rpm-ostree PR merges. I also only compile tested this.

src/rpm_ostree/cli_status.rs Show resolved Hide resolved
src/rpm_ostree/cli_status.rs Outdated Show resolved Hide resolved
src/rpm_ostree/cli_status.rs Outdated Show resolved Hide resolved
src/rpm_ostree/cli_status.rs Show resolved Hide resolved
src/rpm_ostree/cli_status.rs Show resolved Hide resolved
@cgwalters cgwalters force-pushed the rpmostree-client-bindings branch 2 times, most recently from d2eb1bb to c457ecb Compare March 8, 2021 20:25
@cgwalters
Copy link
Member Author

OK rebased 🏄 and cleaned up to use the latest upstream client API. I think this turns out cleaner. Only compile tested though.

@cgwalters cgwalters marked this pull request as ready for review March 8, 2021 20:26
Depends: coreos/rpm-ostree#2636

Use the upstream client bindings for the status data.

An interesting note: In the upstream API, the commit
metadata is exposed as a generic `HashMap<>` because
that's how it's used by both ostree and rpm-ostree.  It'd
be a layering violation for us to hardcode `coreos-assembler.basearch`
in the rpm-ostree git for example, not to mention `fedora-coreos.stream`.
So those constants stay here in zincati.

I dropped the `_json` terminology from various functions
because that's weird - we parsed the data from JSON, but
that's not really very relevant except as an implementation
detail.  It's just a `Deployment`, not a `DeploymentJSON`.

Also I dropped for now the optimization of using `--booted`;
it's not a huge amount of data.  If we care we can re-add that
later.  Note it makes caching more complex because then we
need to carefully check the booted state too.
@cgwalters cgwalters changed the title Rpmostree client bindings rpm-ostree: Use upstream Rust bindings Mar 8, 2021
@cgwalters cgwalters closed this Mar 10, 2021
@cgwalters cgwalters reopened this Mar 10, 2021
@cgwalters
Copy link
Member Author

OK so the cargo publish --dry-run is going to fail with this. Is it really worth publishing zincati to crates.io? It doesn't make sense to cargo install it, etc.

.base_checksum
.clone()
.unwrap_or_else(|| d.checksum.clone()),
version: d.version.clone().unwrap_or_default(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks sketchy, and may break other logic in the app.

I don't know in which cases we can end up with a version-less commit, but it may be good to log a warning and filter it out.

@@ -226,18 +147,4 @@ mod tests {
assert_eq!(deployments.len(), 1);
}
}

#[test]
fn mock_booted_basearch() {
Copy link
Contributor

Choose a reason for hiding this comment

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

These two are some basic unit tests to ensure we don't regress on parsing FCOS-specific metadata, I'd rather not drop it.

@@ -16,6 +17,16 @@ use failure::{ensure, format_err, Fallible, ResultExt};
use serde::Serialize;
use std::cmp::Ordering;

/// Metadata key the base (RPM) architecture; injected by coreos-assembler
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Metadata key the base (RPM) architecture; injected by coreos-assembler
/// Metadata key for the base (RPM) architecture, injected by coreos-assembler.

@lucab
Copy link
Contributor

lucab commented Mar 12, 2021

The crates.io tarball is the main artifact consumed by the Fedora specfile.

We can back away from that by adjusting the release steps. But that would introduce maintenance skew compared to the other (coreos+rust) projects we maintain, and it would still be blocked by cargo package.

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