Skip to content

Commit

Permalink
Make lints stricter, apply crate wide
Browse files Browse the repository at this point in the history
Add `dead_code = "deny"` to our default lints; we had
a compiler warning for this in main.

Fix the warning by moving the human readable test code into
`#[cfg(test)]`.

While we're here, move the other lib.rs lints into the crate;
enforcing docs for *everything* at first I thought might be heavy
handed but actually is fine as it only applies to things that
are `pub`, of which we don't actually have that much so it
mainly forced me to add some stub docs for the modules, which
is probably a good idea.

Signed-off-by: Colin Walters <[email protected]>
  • Loading branch information
cgwalters committed Sep 18, 2024
1 parent ecabb89 commit 7b87222
Show file tree
Hide file tree
Showing 16 changed files with 128 additions and 90 deletions.
10 changes: 2 additions & 8 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,8 @@ jobs:
uses: Swatinem/rust-cache@v2
with:
key: "tests"
- name: cargo fmt (check)
run: cargo fmt -- --check -l
- name: Build
run: cargo test --no-run
- name: Build lib without default features
run: cd lib && cargo check --no-default-features
- name: Individual checks
run: (cd cli && cargo check) && (cd lib && cargo check)
- name: validation
run: make validate
- name: Run tests
run: cargo test -- --nocapture --quiet
- name: Manpage generation
Expand Down
4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ exclude-crate-paths = [ { name = "libz-sys", exclude = "src/zlib" },
unsafe_code = "deny"
# Absolutely must handle errors
unused_must_use = "forbid"
missing_docs = "deny"
missing_debug_implementations = "deny"
# Feel free to comment this one out locally during development of a patch.
dead_code = "deny"

[workspace.lints.clippy]
# These should only be in local code
Expand Down
6 changes: 5 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,14 @@ test-bin-archive: all
test-tmt:
cargo xtask test-tmt

# Checks extra rust things (formatting and select clippy lints)
# Checks extra rust things (formatting, a few extra rust warnings, and select clippy lints)
validate-rust:
cargo fmt -- --check -l
cargo check
(cd lib && cargo check --no-default-features)
cargo test --no-run
cargo clippy -- -D clippy::correctness -D clippy::suspicious
env RUSTDOCFLAGS='-D warnings' cargo doc --lib
.PHONY: validate-rust

validate: validate-rust
Expand Down
3 changes: 3 additions & 0 deletions cli/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
//! The main entrypoint for bootc, which just performs global initialization, and then
//! calls out into the library.

use anyhow::Result;

/// The code called after we've done process global init and created
Expand Down
2 changes: 1 addition & 1 deletion lib/build.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// build.rs
//! Our build script, which basically today just generates a version.

use std::env;
use std::fs;
Expand Down
3 changes: 3 additions & 0 deletions lib/src/boundimage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use camino::Utf8Path;
use cap_std_ext::cap_std::fs::Dir;
use cap_std_ext::dirext::CapStdExtDirExt;
use fn_error_context::context;
#[cfg(feature = "install")]
use ostree_ext::containers_image_proxy;
use ostree_ext::ostree::Deployment;

Expand All @@ -34,6 +35,7 @@ pub(crate) struct BoundImage {
}

#[derive(Debug, PartialEq, Eq)]
#[cfg(feature = "install")]
pub(crate) struct ResolvedBoundImage {
pub(crate) image: String,
pub(crate) digest: String,
Expand Down Expand Up @@ -104,6 +106,7 @@ pub(crate) fn query_bound_images(root: &Dir) -> Result<Vec<BoundImage>> {
Ok(bound_images)
}

#[cfg(feature = "install")]
impl ResolvedBoundImage {
pub(crate) async fn from_image(src: &BoundImage) -> Result<Self> {
let proxy = containers_image_proxy::ImageProxy::new().await?;
Expand Down
2 changes: 1 addition & 1 deletion lib/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@ pub fn global_init() -> Result<()> {
}

/// Parse the provided arguments and execute.
/// Calls [`structopt::clap::Error::exit`] on failure, printing the error message and aborting the program.
/// Calls [`clap::Error::exit`] on failure, printing the error message and aborting the program.
pub async fn run_from_iter<I>(args: I) -> Result<()>
where
I: IntoIterator,
Expand Down
4 changes: 0 additions & 4 deletions lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@
//! to provide a fully "container native" tool for using
//! bootable container images.

// See https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html
#![deny(missing_docs)]
#![deny(missing_debug_implementations)]

mod boundimage;
pub mod cli;
pub(crate) mod deploy;
Expand Down
11 changes: 10 additions & 1 deletion lib/src/lsm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,15 @@ use std::process::Command;

use anyhow::{Context, Result};
use camino::{Utf8Path, Utf8PathBuf};
use cap_std::fs::{Dir, DirBuilder, OpenOptions};
use cap_std::fs::Dir;
#[cfg(feature = "install")]
use cap_std::fs::{DirBuilder, OpenOptions};
#[cfg(feature = "install")]
use cap_std::io_lifetimes::AsFilelike;
use cap_std_ext::cap_std;
#[cfg(feature = "install")]
use cap_std_ext::cap_std::fs::{Metadata, MetadataExt};
#[cfg(feature = "install")]
use cap_std_ext::dirext::CapStdExtDirExt;
use fn_error_context::context;
#[cfg(feature = "install")]
Expand Down Expand Up @@ -212,13 +217,15 @@ pub(crate) fn set_security_selinux(fd: std::os::fd::BorrowedFd, label: &[u8]) ->

/// The labeling state; "unsupported" is distinct as we need to handle
/// cases like the ESP which don't support labeling.
#[cfg(feature = "install")]
pub(crate) enum SELinuxLabelState {
Unlabeled,
Unsupported,
Labeled,
}

/// Query the SELinux labeling for a particular path
#[cfg(feature = "install")]
pub(crate) fn has_security_selinux(root: &Dir, path: &Utf8Path) -> Result<SELinuxLabelState> {
// TODO: avoid hardcoding a max size here
let mut buf = [0u8; 2048];
Expand All @@ -231,6 +238,7 @@ pub(crate) fn has_security_selinux(root: &Dir, path: &Utf8Path) -> Result<SELinu
}
}

#[cfg(feature = "install")]
pub(crate) fn set_security_selinux_path(root: &Dir, path: &Utf8Path, label: &[u8]) -> Result<()> {
// TODO: avoid hardcoding a max size here
let fdpath = format!("/proc/self/fd/{}/", root.as_raw_fd());
Expand All @@ -244,6 +252,7 @@ pub(crate) fn set_security_selinux_path(root: &Dir, path: &Utf8Path, label: &[u8
Ok(())
}

#[cfg(feature = "install")]
pub(crate) fn ensure_labeled(
root: &Dir,
path: &Utf8Path,
Expand Down
11 changes: 9 additions & 2 deletions lib/src/podman.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
use anyhow::{anyhow, Result};
#[cfg(feature = "install")]
use anyhow::Result;
#[cfg(feature = "install")]
use camino::Utf8Path;
#[cfg(feature = "install")]
use cap_std_ext::cap_std::fs::Dir;
use serde::Deserialize;

/// Where we look inside our container to find our own image
/// for use with `bootc install`.
#[cfg(feature = "install")]
pub(crate) const CONTAINER_STORAGE: &str = "/var/lib/containers";

#[derive(Deserialize)]
#[serde(rename_all = "PascalCase")]
#[cfg(feature = "install")]
pub(crate) struct Inspect {
pub(crate) digest: String,
}
Expand All @@ -31,11 +36,12 @@ pub(crate) fn imageid_to_digest(imgid: &str) -> Result<String> {
let i = o
.into_iter()
.next()
.ok_or_else(|| anyhow!("No images returned for inspect"))?;
.ok_or_else(|| anyhow::anyhow!("No images returned for inspect"))?;
Ok(i.digest)
}

/// Return true if there is apparently an active container store at the target path.
#[cfg(feature = "install")]
pub(crate) fn storage_exists(root: &Dir, path: impl AsRef<Utf8Path>) -> Result<bool> {
fn impl_storage_exists(root: &Dir, path: &Utf8Path) -> Result<bool> {
let lock = "storage.lock";
Expand All @@ -49,6 +55,7 @@ pub(crate) fn storage_exists(root: &Dir, path: impl AsRef<Utf8Path>) -> Result<b
///
/// Note this does not attempt to parse the root filesystem's container storage configuration,
/// this uses a hardcoded default path.
#[cfg(feature = "install")]
pub(crate) fn storage_exists_default(root: &Dir) -> Result<bool> {
storage_exists(root, CONTAINER_STORAGE.trim_start_matches('/'))
}
147 changes: 77 additions & 70 deletions lib/src/status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ pub(crate) struct Deployments {
pub(crate) other: VecDeque<ostree::Deployment>,
}

#[cfg(feature = "install")]
pub(crate) fn try_deserialize_timestamp(t: &str) -> Option<chrono::DateTime<chrono::Utc>> {
match chrono::DateTime::parse_from_rfc3339(t).context("Parsing timestamp") {
Ok(t) => Some(t.into()),
Expand Down Expand Up @@ -362,20 +363,24 @@ fn human_readable_output(mut out: impl Write, host: &Host) -> Result<()> {
Ok(())
}

fn human_status_from_spec_fixture(spec_fixture: &str) -> Result<String> {
let host: Host = serde_yaml::from_str(spec_fixture).unwrap();
let mut w = Vec::new();
human_readable_output(&mut w, &host).unwrap();
let w = String::from_utf8(w).unwrap();
Ok(w)
}
#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_human_readable_base_spec() {
// Tests Staged and Booted, null Rollback
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-booted.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
fn human_status_from_spec_fixture(spec_fixture: &str) -> Result<String> {
let host: Host = serde_yaml::from_str(spec_fixture).unwrap();
let mut w = Vec::new();
human_readable_output(&mut w, &host).unwrap();
let w = String::from_utf8(w).unwrap();
Ok(w)
}

#[test]
fn test_human_readable_base_spec() {
// Tests Staged and Booted, null Rollback
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-booted.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
Current staged image: quay.io/example/someimage:latest
Image version: nightly (2023-10-14 19:22:15 UTC)
Image transport: registry
Expand All @@ -386,61 +391,62 @@ fn test_human_readable_base_spec() {
Image digest: sha256:736b359467c9437c1ac915acaae952aad854e07eb4a16a94999a48af08c83c34
No rollback image present
"};
similar_asserts::assert_eq!(w, expected);
}
similar_asserts::assert_eq!(w, expected);
}

#[test]
fn test_human_readable_rfe_spec() {
// Basic rhel for edge bootc install with nothing
let w =
human_status_from_spec_fixture(include_str!("fixtures/spec-rfe-ostree-deployment.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
#[test]
fn test_human_readable_rfe_spec() {
// Basic rhel for edge bootc install with nothing
let w = human_status_from_spec_fixture(include_str!(
"fixtures/spec-rfe-ostree-deployment.yaml"
))
.expect("No spec found");
let expected = indoc::indoc! { r"
Current staged state is native ostree
Current booted state is native ostree
No rollback image present
"};
similar_asserts::assert_eq!(w, expected);
}
similar_asserts::assert_eq!(w, expected);
}

#[test]
fn test_human_readable_staged_spec() {
// staged image, no boot/rollback
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-ostree-to-bootc.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
#[test]
fn test_human_readable_staged_spec() {
// staged image, no boot/rollback
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-ostree-to-bootc.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
Current staged image: quay.io/centos-bootc/centos-bootc:stream9
Image version: stream9.20240807.0 (No timestamp present)
Image transport: registry
Image digest: sha256:47e5ed613a970b6574bfa954ab25bb6e85656552899aa518b5961d9645102b38
Current booted state is native ostree
No rollback image present
"};
similar_asserts::assert_eq!(w, expected);
}
similar_asserts::assert_eq!(w, expected);
}

#[test]
fn test_human_readable_booted_spec() {
// booted image, no staged/rollback
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-only-booted.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
#[test]
fn test_human_readable_booted_spec() {
// booted image, no staged/rollback
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-only-booted.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
No staged image present
Current booted image: quay.io/centos-bootc/centos-bootc:stream9
Image version: stream9.20240807.0 (No timestamp present)
Image transport: registry
Image digest: sha256:47e5ed613a970b6574bfa954ab25bb6e85656552899aa518b5961d9645102b38
No rollback image present
"};
similar_asserts::assert_eq!(w, expected);
}
similar_asserts::assert_eq!(w, expected);
}

#[test]
fn test_human_readable_staged_rollback_spec() {
// staged/rollback image, no booted
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-rollback.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
#[test]
fn test_human_readable_staged_rollback_spec() {
// staged/rollback image, no booted
let w = human_status_from_spec_fixture(include_str!("fixtures/spec-staged-rollback.yaml"))
.expect("No spec found");
let expected = indoc::indoc! { r"
Current staged image: quay.io/example/someimage:latest
Image version: nightly (2023-10-14 19:22:15 UTC)
Image transport: registry
Expand All @@ -451,29 +457,30 @@ fn test_human_readable_staged_rollback_spec() {
Image transport: registry
Image digest: sha256:736b359467c9437c1ac915acaae952aad854e07eb4a16a94999a48af08c83c34
"};
similar_asserts::assert_eq!(w, expected);
}
similar_asserts::assert_eq!(w, expected);
}

#[test]
fn test_convert_signatures() {
use std::str::FromStr;
let ir_unverified = &OstreeImageReference::from_str(
"ostree-unverified-registry:quay.io/someexample/foo:latest",
)
.unwrap();
let ir_ostree = &OstreeImageReference::from_str(
"ostree-remote-registry:fedora:quay.io/fedora/fedora-coreos:stable",
)
.unwrap();

let ir = ImageReference::from(ir_unverified.clone());
assert_eq!(ir.image, "quay.io/someexample/foo:latest");
assert_eq!(ir.signature, None);

let ir = ImageReference::from(ir_ostree.clone());
assert_eq!(ir.image, "quay.io/fedora/fedora-coreos:stable");
assert_eq!(
ir.signature,
Some(ImageSignature::OstreeRemote("fedora".into()))
);
#[test]
fn test_convert_signatures() {
use std::str::FromStr;
let ir_unverified = &OstreeImageReference::from_str(
"ostree-unverified-registry:quay.io/someexample/foo:latest",
)
.unwrap();
let ir_ostree = &OstreeImageReference::from_str(
"ostree-remote-registry:fedora:quay.io/fedora/fedora-coreos:stable",
)
.unwrap();

let ir = ImageReference::from(ir_unverified.clone());
assert_eq!(ir.image, "quay.io/someexample/foo:latest");
assert_eq!(ir.signature, None);

let ir = ImageReference::from(ir_ostree.clone());
assert_eq!(ir.image, "quay.io/fedora/fedora-coreos:stable");
assert_eq!(
ir.signature,
Some(ImageSignature::OstreeRemote("fedora".into()))
);
}
}
Loading

0 comments on commit 7b87222

Please sign in to comment.