decrating: absorb shipper-environment into shipper::runtime::environment#65
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (20)
✨ Finishing Touches🧪 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: 4e002fd33b
ℹ️ 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".
| } | ||
| } | ||
| } | ||
| pub use shipper_storage::{CloudStorageConfig, StorageBackend, StorageType}; |
There was a problem hiding this comment.
Keep
StorageBackend API compatible with prior trait
Re-exporting shipper_storage::StorageBackend here changes the public trait contract: it now requires list (and inherits new default methods), whereas shipper::storage::StorageBackend previously had no list requirement. Any downstream crate that implemented the old trait will fail to compile after this change, which contradicts the shim’s stated goal of preserving the existing API surface.
Useful? React with 👍 / 👎.
Stacks on PR #53 (env+storage+git dual-impl cleanup). Move the 2202-LOC shipper-environment crate into crates/shipper/src/runtime/environment/ as crate-private modules (mod.rs + fingerprint.rs + ci.rs, ~2190 LOC total). Preserves the deduped in-tree wrapper logic (normalize_version, EnvironmentInfo::collect delegation with graceful fallback) from PR #53. Split seams: - mod.rs — CiEnvironment, detect_environment, is_ci, collect_environment_fingerprint (the absorbed shim), CiEnvironment snapshot tests. - fingerprint.rs — EnvironmentInfo, get_rust_version, get_cargo_version, get_environment_fingerprint, normalize_tool_version, collect_env_vars, tests + proptests. - ci.rs — get_ci_branch, get_ci_commit_sha, is_pull_request, provider-specific tests. All 13 snapshots preserved (renamed to `shipper__runtime__environment__ snapshot_tests__*.snap`). All 130 absorbed tests pass in-tree. Surgical unblock (precedent: PR #54 policy→engine-parallel inline): shipper-state depended on shipper-environment via one call site in migrate_v1_to_v2. Because shipper-state is an (optional) dep of shipper, it cannot depend on shipper itself — a dep cycle. Inlined an ~20-LOC collect_environment_fingerprint into shipper-state (marked `#[doc(hidden)]`, with a comment pointing back at crate::runtime::environment for the post-shipper-state-absorption replacement), and dropped shipper-environment from shipper-state's Cargo.toml. Will be replaced with a direct call to crate::runtime::environment::collect_environment_fingerprint after shipper-state is itself absorbed into shipper::state. Module is `pub(crate) mod runtime; mod environment;` — the former public `shipper::environment` API had no external consumers in-tree and is now crate-private per the runtime-layer discipline. `#[allow(dead_code)]` is applied at the runtime::environment level because several absorbed items (CI branch/SHA/PR helpers, the short pipe-fingerprint form, EnvironmentInfo::fingerprint) currently have no in-crate callers but retain full test coverage for future wiring. Also updates shipper's `insta` dev-dep to enable the `yaml` feature (required by the moved snapshot_tests module). Per docs/decrating-plan.md section 6 Phase 2.
4e002fd to
7bf17f0
Compare
Summary
Absorbs the 2202-LOC
shipper-environmentmicrocrate intocrates/shipper/src/runtime/environment/as crate-private modules (Phase 2 decrating).Depends on PR #53 (dual-implementation cleanup for env + storage + git) being merged first — this PR stacks on
feature/decrating-phase1-medium.Split seams (~2190 LOC across 3 files):
mod.rs—CiEnvironment,detect_environment,is_ci,collect_environment_fingerprint(the absorbed PR decrating: Phase 1 (medium batch) — collapse dual implementations for environment/storage/git #53 shim with fallback +normalize_version).fingerprint.rs—EnvironmentInfo+collect,get_rust_version,get_cargo_version,get_environment_fingerprint,normalize_tool_version,collect_env_vars, tests + proptests.ci.rs—get_ci_branch,get_ci_commit_sha,is_pull_request, per-provider tests.All 13
instasnapshots preserved (renamed fromshipper_environment__snapshot_tests__*toshipper__runtime__environment__snapshot_tests__*). All 130 absorbed tests pass in-tree.Surgical unblock (precedent: PR #54 policy → engine-parallel inline)
shipper-statedepended onshipper-environmentvia one call site inmigrate_v1_to_v2. Becauseshipper-stateis an (optional) dep ofshipper, it cannot depend onshipperitself — a dep cycle. This PR inlines a ~20-LOCcollect_environment_fingerprintintoshipper-state(marked#[doc(hidden)], with a TODO comment pointing atcrate::runtime::environment) and dropsshipper-environmentfromshipper-state/Cargo.toml. The inline will be replaced with a direct call aftershipper-stateis itself absorbed intoshipper::state.API discipline
pub(crate) mod runtime; mod environment;— the former publicshipper::environmentAPI had no external consumers in-tree, so it goes crate-private per runtime-layer rules.#[allow(dead_code)]at theruntime::environmentlevel: several absorbed items (CI branch/SHA/PR helpers, the short pipe-fingerprint form,EnvironmentInfo::fingerprint) have no in-crate callers yet but keep full test coverage for future wiring.shipper'sinstadev-dep is bumped tofeatures = ["yaml"](required by the movedsnapshot_testsmodule).Per
docs/decrating-plan.md§6 Phase 2.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspacecargo test -p shipper-state(39 passed — confirms surgical unblock)cargo test -p shipper --lib(854 passed)cargo test -p shipper --lib runtime::environment(130 passed — absorbed suite)cargo test -p shipper --tests(34 integration + 2 schema passed)cargo test -p shipper-cli --bins(16 passed)mainafter PR decrating: Phase 1 (medium batch) — collapse dual implementations for environment/storage/git #53 merges