feat: rust cli - #551
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
rust/crates/adc-cli/src/progress.rs (2)
159-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd unit tests for
compact_duration.
compact_durationis pure and covers three branches plus the boundaries at 60 s and 3600 s.sync_slotsandsync_reportboth render its output. A small test locks the format.💚 Proposed test
#[test] fn compact_duration_formats_each_branch() { use std::time::Duration; assert_eq!(compact_duration(Duration::from_secs(0)), "0s"); assert_eq!(compact_duration(Duration::from_secs(59)), "59s"); assert_eq!(compact_duration(Duration::from_secs(60)), "1m00s"); assert_eq!(compact_duration(Duration::from_secs(3599)), "59m59s"); assert_eq!(compact_duration(Duration::from_secs(3600)), "1h00m"); assert_eq!(compact_duration(Duration::from_secs(7380)), "2h03m"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-cli/src/progress.rs` around lines 159 - 186, Add a unit test in the existing tests module covering compact_duration’s seconds, minutes, and hours branches, including the 60-second and 3600-second boundaries and representative formatting such as zero-padding and multi-hour output. Use Duration inputs and assert the exact expected strings.
128-156: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache the
is_terminal()result.Each formatted line calls
std::io::stderr().is_terminal()twice: once informat_scoped_lineand once incolored_icon_and_label. At--verbose 2a large sync produces one block per event plus one per HTTP request, so this repeats a syscall on a hot path. The value cannot change during a run. Cache it once.♻️ Proposed change
+fn stderr_is_tty() -> bool { + static IS_TTY: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); + *IS_TTY.get_or_init(|| std::io::stderr().is_terminal()) +} + pub fn format_scoped_line(scope: &str, icon: char, label: &str, message: &str) -> String { let now = chrono::Local::now().format("%I:%M:%S %p"); let meta = format!("[{now}] [{scope}] \u{203a}"); - let meta = if std::io::stderr().is_terminal() { + let meta = if stderr_is_tty() { format!("\u{1b}[90m{meta}\u{1b}[0m") } else { meta }; @@ fn colored_icon_and_label(icon: char, label: &str) -> String { let padded = format!("{label:<10}"); - if !std::io::stderr().is_terminal() { + if !stderr_is_tty() { return format!("{icon} {padded}"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-cli/src/progress.rs` around lines 128 - 156, Cache the stderr terminal-status result once in format_scoped_line and pass it into colored_icon_and_label, removing the second is_terminal() call. Use the cached value for both metadata and icon/label coloring while preserving the existing terminal and non-terminal formatting.rust/crates/adc-cli/src/logging/sync_report.rs (1)
108-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated percent and ETA computation.
print_progressrepeats the percent and ETA math fromsync_slots::render(rust/crates/adc-cli/src/logging/sync_slots.rslines 105-116). The two copies can drift. Extract one helper, for example inprogress.rsnext tocompact_duration.♻️ Proposed helper
// rust/crates/adc-cli/src/progress.rs /// Completion percentage and ETA for `completed`/`total` after `elapsed`. pub fn percent_and_eta(completed: u64, total: u64, elapsed: std::time::Duration) -> (u64, std::time::Duration) { let percent = completed .checked_mul(100) .and_then(|n| n.checked_div(total)) .unwrap_or(100); let eta = if completed > 0 { let secs_per_event = elapsed.as_secs_f64() / completed as f64; std::time::Duration::from_secs_f64(secs_per_event * total.saturating_sub(completed) as f64) } else { std::time::Duration::ZERO }; (percent, eta) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-cli/src/logging/sync_report.rs` around lines 108 - 136, Extract the shared percent and ETA calculation from print_progress and sync_slots::render into a single helper near compact_duration in progress.rs, preserving the current fallback and zero-completion behavior. Update both callers to use the helper and remove their duplicated arithmetic.rust/crates/adc-cli/src/logging/mod.rs (1)
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider honoring
RUST_LOGas an override.
EnvFilter::new(log_filter)ignores the environment.--verbosebecomes the only control, so users cannot raise the level for one crate during debugging.EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new(log_filter))keeps the current defaults and adds the override.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-cli/src/logging/mod.rs` around lines 50 - 58, Update init’s EnvFilter construction to first use the RUST_LOG value via try_from_default_env, falling back to EnvFilter::new(log_filter) when the environment variable is absent or invalid; preserve the existing verbose-based log_filter defaults.rust/crates/adc-cli/src/pipeline.rs (1)
26-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail before creating the client when
args.tokenis absent.unwrap_or_default()sends an emptyX-API-KEYand delays the error until the backend rejects the request. Return a clear error that names--tokenandADC_TOKEN.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-cli/src/pipeline.rs` around lines 26 - 36, Update the client setup in the pipeline flow to validate that args.token is present before constructing HttpClient, instead of using unwrap_or_default(). Return a clear error identifying both --token and ADC_TOKEN when the token is absent, while preserving the existing token value for configured requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/crates/adc-backend-apisix/src/operator.rs`:
- Around line 188-193: Update is_retriable to apply the APISIX “is still using
it now” exception only when BackendError::Api has status 400, while preserving
normal err.is_retriable() behavior. Add a test covering the same message with a
non-400 status and assert it is not retriable.
In `@rust/crates/adc-backend-core/src/client.rs`:
- Around line 163-178: Redact sensitive JSON fields before recording
request_body and response_body in the execute flow around recording_span, and
add coverage for consumer credentials and plugin configuration. In
rust/crates/adc-backend-core/src/client.rs:163-178, update the body-recording
path to use the redacted representation; in
rust/crates/adc-backend-core/src/client.rs:267-279, update the header formatting
helper to redact recognized Authorization, Cookie, and Set-Cookie names
regardless of HeaderValue::is_sensitive(), with corresponding coverage.
- Around line 169-189: Update the response handling in the Ok(response) branch
to propagate a classified error when response.bytes() fails instead of
defaulting to an empty body. Preserve response metadata when rebuilding by using
ResponseBuilderExt::url(response.url().clone()) and setting .version(version),
while retaining the existing status, headers, and successful body behavior.
In `@rust/crates/adc-cli/src/cli.rs`:
- Around line 35-40: Remove or hide the Convert, IngressSync, and IngressServer
variants from the CLI command definitions so they no longer appear in help while
their handlers in main remain unimplemented. Update the associated command
dispatch or parsing code as needed to keep the CLI compiling and preserve all
implemented subcommands.
- Around line 224-229: Update existing_file to validate the path with is_file()
instead of exists(), so directories and other non-regular paths are rejected
immediately while valid files still return Ok(path).
In `@rust/crates/adc-cli/src/config.rs`:
- Around line 97-126: Update the recognized resource-key handling in the merge
logic around ARRAY_KEYS and MAP_KEYS so invalid value types return CliError
instead of defaulting to empty collections. Replace the unwrap_or_default
behavior for val.as_array() and val.as_object() with validation that reports the
configuration path and affected key, while preserving normal merging for valid
arrays and objects.
- Around line 150-159: Update the SNI normalization branch in the configuration
key generation logic to sort the values collected from `snis` before joining
them. Preserve the existing string filtering and comma-separated key format so
equivalent SNI sets produce the same duplicate-detection key regardless of input
order.
- Around line 39-42: Update the glob collection in the surrounding
configuration-loading function to stop using filter_map(Result::ok), propagate
every GlobError instead, and include the source pattern in the propagated error.
Preserve filtering to files and successful match collection while ensuring any
traversal failure aborts configuration loading.
- Around line 226-248: The strip_ids function only removes IDs from array
resources; extend it to process the global_rules and plugin_metadata object maps
as well. Iterate their map values and call remove_id on each value, while
preserving the existing handling for services, ssls, consumers, and
consumer_groups.
- Around line 257-296: Update filter_resource_types to handle nested resource
types such as Route and Upstream under config.services instead of treating only
top-level buckets as filterable. Ensure include route preserves services while
filtering nested resources to routes, and exclude route removes routes from each
service; alternatively, reject unsupported nested resource types consistently
before applying the filter.
In `@rust/crates/adc-cli/src/logging/sync_slots.rs`:
- Around line 166-186: Update on_close so the ACTIVE mutex guard is released
before any terminal output: under the lock, update counters and copy the
required display data into local values, then drop the guard; afterward perform
multi.println and render using those copied values. Refactor render or introduce
a render_frame helper as needed so formatting occurs without requiring the
ACTIVE lock, while preserving the existing success and failure output.
In `@rust/crates/adc-cli/src/main.rs`:
- Around line 104-107: Correct the user-facing println! message in the main flow
after writing diff.yaml, changing the grammatically incorrect “has been wrote”
wording to “has been written” while preserving the existing output and
file-writing behavior.
- Around line 158-175: Update the summary calculation near the results loop to
report only successful events as applied by subtracting failed from
results.len(). Keep the existing failed count and progress::info output
unchanged.
---
Nitpick comments:
In `@rust/crates/adc-cli/src/logging/mod.rs`:
- Around line 50-58: Update init’s EnvFilter construction to first use the
RUST_LOG value via try_from_default_env, falling back to
EnvFilter::new(log_filter) when the environment variable is absent or invalid;
preserve the existing verbose-based log_filter defaults.
In `@rust/crates/adc-cli/src/logging/sync_report.rs`:
- Around line 108-136: Extract the shared percent and ETA calculation from
print_progress and sync_slots::render into a single helper near compact_duration
in progress.rs, preserving the current fallback and zero-completion behavior.
Update both callers to use the helper and remove their duplicated arithmetic.
In `@rust/crates/adc-cli/src/pipeline.rs`:
- Around line 26-36: Update the client setup in the pipeline flow to validate
that args.token is present before constructing HttpClient, instead of using
unwrap_or_default(). Return a clear error identifying both --token and ADC_TOKEN
when the token is absent, while preserving the existing token value for
configured requests.
In `@rust/crates/adc-cli/src/progress.rs`:
- Around line 159-186: Add a unit test in the existing tests module covering
compact_duration’s seconds, minutes, and hours branches, including the 60-second
and 3600-second boundaries and representative formatting such as zero-padding
and multi-hour output. Use Duration inputs and assert the exact expected
strings.
- Around line 128-156: Cache the stderr terminal-status result once in
format_scoped_line and pass it into colored_icon_and_label, removing the second
is_terminal() call. Use the cached value for both metadata and icon/label
coloring while preserving the existing terminal and non-terminal formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eed2347-6dd6-483f-9a51-d34c19d28b02
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.github/workflows/unit.yamlrust/Cargo.tomlrust/crates/adc-backend-apisix/Cargo.tomlrust/crates/adc-backend-apisix/src/backend.rsrust/crates/adc-backend-apisix/src/fetcher.rsrust/crates/adc-backend-apisix/src/operator.rsrust/crates/adc-backend-core/Cargo.tomlrust/crates/adc-backend-core/src/client.rsrust/crates/adc-backend-core/src/lib.rsrust/crates/adc-backend-core/src/retry.rsrust/crates/adc-backend-core/tests/retry.rsrust/crates/adc-cli/Cargo.tomlrust/crates/adc-cli/src/cli.rsrust/crates/adc-cli/src/config.rsrust/crates/adc-cli/src/error.rsrust/crates/adc-cli/src/logging/http_debug.rsrust/crates/adc-cli/src/logging/mod.rsrust/crates/adc-cli/src/logging/sync_debug.rsrust/crates/adc-cli/src/logging/sync_report.rsrust/crates/adc-cli/src/logging/sync_slots.rsrust/crates/adc-cli/src/logging/sync_span_fields.rsrust/crates/adc-cli/src/main.rsrust/crates/adc-cli/src/pipeline.rsrust/crates/adc-cli/src/progress.rsrust/crates/adc-sdk/src/backend/error.rsrust/crates/adc-sdk/src/backend/mod.rsrust/crates/adc-sdk/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/crates/adc-cli/src/progress.rs`:
- Around line 184-187: Update the percentage calculation in the progress
reporting logic to perform the multiplication and division in a wider integer
type, avoiding overflow when completed is below total. Clamp the resulting
percentage to 100 so completed values above total cannot exceed 100%, and add
regression tests covering both an overflowing product and completed greater than
total.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6d8948f-d7a3-47f3-b821-8024b0782c20
📒 Files selected for processing (10)
rust/crates/adc-backend-apisix/src/operator.rsrust/crates/adc-backend-core/src/client.rsrust/crates/adc-cli/src/cli.rsrust/crates/adc-cli/src/config.rsrust/crates/adc-cli/src/logging/mod.rsrust/crates/adc-cli/src/logging/sync_report.rsrust/crates/adc-cli/src/logging/sync_slots.rsrust/crates/adc-cli/src/main.rsrust/crates/adc-cli/src/pipeline.rsrust/crates/adc-cli/src/progress.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- rust/crates/adc-cli/src/logging/mod.rs
- rust/crates/adc-backend-apisix/src/operator.rs
- rust/crates/adc-cli/src/logging/sync_slots.rs
- rust/crates/adc-backend-core/src/client.rs
- rust/crates/adc-cli/src/pipeline.rs
- rust/crates/adc-cli/src/config.rs
- rust/crates/adc-cli/src/main.rs
- rust/crates/adc-cli/src/cli.rs
This was referenced Aug 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add cli interface.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Diagnostics