decrating: absorb shipper-cargo into shipper::ops::cargo#74
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (17)
Summary by CodeRabbit
WalkthroughThe pull request removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c02070671
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| default = ["micro-all"] | ||
| micro-encrypt = ["shipper/micro-encrypt"] | ||
| micro-cargo = ["shipper/micro-cargo"] | ||
| micro-webhook = ["shipper/micro-webhook"] | ||
| micro-types = ["shipper/micro-types"] | ||
| micro-config = ["shipper/micro-config"] |
There was a problem hiding this comment.
Restore
micro-cargo feature for BDD matrix compatibility
Removing the micro-cargo entry from shipper-cli features makes the existing compatibility matrix fail immediately: CI still runs cargo test -p shipper-cli --test bdd_publish --features micro-cargo in .github/workflows/ci.yml (lines 217 and 233–238) and the CircleCI template does the same (templates/circleci-config.yml:51-56), but this commit now errors with the package 'shipper-cli' does not contain this feature: micro-cargo. This turns one matrix leg into a guaranteed failure until the alias is kept (possibly deprecated/no-op) or the matrix is updated in the same change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request absorbs the shipper-cargo microcrate into the main shipper crate as crate::ops::cargo, consolidating workspace metadata and publishing logic. Feedback focuses on improving the migrated code by ensuring is_publishable correctly filters for workspace members, eliminating logic duplication in metadata loading, and optimizing performance in dependency graph construction and package name validation.
| pub fn is_publishable(&self, package: &Package) -> bool { | ||
| if let Some(publish) = &package.publish | ||
| && publish.is_empty() | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if package.version.to_string() == "0.0.0" { | ||
| return false; | ||
| } | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
is_publishable should verify that the package is a member of the workspace. Without this check, publishable_packages() and topological_order() will include all public dependencies (like serde or anyhow) found in the metadata, which is incorrect for workspace publishing operations.
pub fn is_publishable(&self, package: &Package) -> bool {
if !self.metadata.workspace_members.contains(&package.id) {
return false;
}
if let Some(publish) = &package.publish
&& publish.is_empty()
{
return false;
}
if package.version.to_string() == "0.0.0" {
return false;
}
true
}| let metadata = MetadataCommand::new() | ||
| .manifest_path(manifest_path) | ||
| .exec() | ||
| .context("failed to load cargo metadata")?; |
There was a problem hiding this comment.
This logic duplicates the load_metadata function defined at line 170. It's better to call load_metadata(manifest_path) directly to ensure consistent behavior and error handling.
| let metadata = MetadataCommand::new() | |
| .manifest_path(manifest_path) | |
| .exec() | |
| .context("failed to load cargo metadata")?; | |
| let metadata = load_metadata(manifest_path)?; |
| .packages | ||
| .iter() | ||
| .find(|p| p.name == dep.name) | ||
| .map(|p| p.name.to_string()) |
There was a problem hiding this comment.
Performing a linear search for each dependency in self.metadata.packages results in poor performance (HashMap<String, &Package>) once at the start of the function.
| let chars: Vec<char> = name.chars().collect(); | ||
|
|
||
| if chars[0].is_ascii_digit() || chars[0] == '-' { | ||
| return false; | ||
| } | ||
|
|
||
| chars | ||
| .iter() | ||
| .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || *c == '-' || *c == '_') | ||
| } |
There was a problem hiding this comment.
Collecting all characters into a Vec<char> is an unnecessary allocation. You can check the first character and validate the rest of the string using iterators directly on the &str.
| let chars: Vec<char> = name.chars().collect(); | |
| if chars[0].is_ascii_digit() || chars[0] == '-' { | |
| return false; | |
| } | |
| chars | |
| .iter() | |
| .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || *c == '-' || *c == '_') | |
| } | |
| let mut chars = name.chars(); | |
| match chars.next() { | |
| None => return false, | |
| Some(c) if c.is_ascii_digit() || c == '-' => return false, | |
| _ => {} | |
| } | |
| name.chars() | |
| .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_') |
2c02070 to
a6b052a
Compare
The former `shipper-cargo` microcrate is pulled into the core `shipper` crate as `crate::ops::cargo`, following the Phase-2 decrating plan (`docs/decrating-plan.md` §3.2). The historical public path `shipper::cargo` is preserved via `pub use crate::ops::cargo` for backward compatibility with `shipper-cli` and external consumers. Scope of changes: - New module `crates/shipper/src/ops/cargo/` containing merged functionality of the absorbed crate PLUS the pre-existing `crates/shipper/src/cargo.rs` (the `micro-cargo`-off branch's local implementation). The merged module exposes `CargoOutput`, `cargo_publish` / dry-run variants, `load_metadata`, `WorkspaceMetadata`, `PackageInfo`, `is_valid_package_name`, `workspace_member_names`, `get_version`, `get_package_name`, plus a `pub use shipper_output_sanitizer::redact_sensitive` re-export. All tests (unit, proptest, snapshot) from both the absorbed crate and the in-tree duplicate are ported; snapshot filenames rewritten to match the new module path. - `shipper::cargo` is now `pub use crate::ops::cargo;` — no more `#[cfg(feature = "micro-cargo")]` dispatch. The `micro-cargo` feature is removed from `crates/shipper/Cargo.toml` and `crates/shipper-cli/Cargo.toml`. - `crates/shipper/src/engine/parallel/publish.rs` swaps `use shipper_cargo as cargo;` for `use crate::ops::cargo;`. - `crates/shipper/src/plan/mod.rs::load_metadata` now delegates to `crate::ops::cargo::load_metadata`. - `crates/shipper-cargo/` physically deleted; dropped from workspace members in the root `Cargo.toml` and from `[dependencies]` in `crates/shipper/Cargo.toml`. - Engine-parallel CLAUDE.md updated to reflect the new internal import. - New `crates/shipper/src/ops/cargo/CLAUDE.md` describing invariants and gotchas of the absorbed module, matching the convention used by `ops/auth`, `ops/lock`, `ops/process`. Pre-flight (per R-PR-4): the only consumer of `shipper-cargo` on main was `crates/shipper/Cargo.toml` itself; `fuzz/` only references the distinct `shipper-cargo-failure` crate. After this change the `shipper_cargo::` namespace is gone from the workspace entirely. Verified locally: - `cargo check --workspace --all-targets` — clean - `cargo check --workspace --all-targets --all-features` — clean - `cargo clippy --workspace --all-targets --all-features -- -D warnings` — clean - `cargo test --workspace` — 2000+ tests pass. One pre-existing `preflight_allow_dirty_snapshot` flake in `shipper-cli` was confirmed to fail identically on `origin/main` without these changes, so it is not caused by the absorption. Part of the decrating effort — unblocks the next absorption round now that `shipper-cargo` is off the dependency list of every crate other than `shipper`.
a6b052a to
6c94851
Compare
Summary
Phase-2 decrating: absorb the
shipper-cargomicrocrate into the coreshippercrate ascrate::ops::cargo. Public pathshipper::cargois preserved viapub usefor backward compatibility — no breaking changes forshipper-clior external consumers of the library.crates/shipper/src/cargo.rs(the localmicro-cargo-off implementation) andcrates/shipper-cargo/src/lib.rs(the absorbed crate) are merged into one module atcrates/shipper/src/ops/cargo/mod.rs. The merged surface includesCargoOutput,cargo_publish(+ dry-run variants),load_metadata,WorkspaceMetadata,PackageInfo,is_valid_package_name,workspace_member_names,get_version,get_package_name, plus apub use shipper_output_sanitizer::redact_sensitivere-export.micro-cargofeature is removed from bothcrates/shipper/Cargo.tomlandcrates/shipper-cli/Cargo.toml;shipper::cargobecomes an unconditionalpub use crate::ops::cargo.crates/shipper/src/engine/parallel/publish.rs—use shipper_cargo as cargo;→use crate::ops::cargo;crates/shipper/src/plan/mod.rs::load_metadata— delegates tocrate::ops::cargo::load_metadatacrates/shipper-cargo/physically deleted; dropped from[workspace.members]in the rootCargo.tomland from[dependencies]incrates/shipper/Cargo.toml.crates/shipper/src/ops/cargo/CLAUDE.mddocumenting invariants and gotchas, matching the convention used byops/auth,ops/lock,ops/process.Pre-flight (per R-PR-4): on this base, the only consumer of
shipper-cargowascrates/shipper/Cargo.tomlitself.fuzz/only references the distinctshipper-cargo-failurecrate. After this PR theshipper_cargo::namespace is gone from the workspace entirely.Test plan
Verified locally before pushing (per R-PR-5):
cargo check --workspace --all-targets— cleancargo check --workspace --all-targets --all-features— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --workspace— 2000+ tests pass. One pre-existing snapshot flake (preflight_allow_dirty_snapshotinshipper-cli) was confirmed to fail identically onorigin/mainwithout these changes; it is a localhost-mock-registry timing issue unrelated to this absorption.CI checks to watch
cargo checkmatrix passescargo testworkspace (thepreflight_allow_dirty_snapshotflake may surface — pre-existing)cargo clippy -- -D warningsops/cargois a layer-1 module; it imports onlycrate::ops::process,crate::types-equivalent external types, and external crates — no upward imports)