refactor(cli): extract shared helpers into commands/common module#2359
refactor(cli): extract shared helpers into commands/common module#2359varshaprasad96 wants to merge 2 commits into
Conversation
|
I'm curious about the decision for placing common.rs under commands/ rather than at the top level of src/. Since run.rs (which is at the parent level) imports from it, and the utilities seem fairly general-purpose, I'm wondering if src/common.rs might be cleaner? |
|
@2000krysztof - The fact that run.rs currently imports from commands/common is a transitional state, Placing it at src/common.rs would make it a peer of auth.rs, ssh.rs, tls.rs which are infrastructure modules, a different layer than command-specific helpers. |
|
Makes sense, thanks for the explanation. Looks good to me. |
BlockedHead SHA: Gator is blocked because this PR currently has merge conflicts with Next action: @varshaprasad96, please update the branch from |
Move shared formatting, parsing, display, and settings helpers from run.rs into a new commands/common.rs module. run.rs re-exports all public items so no callers change. This establishes the commands/ directory structure for the incremental run.rs split (NVIDIA#2304). Signed-off-by: Varsha Prasad Narsing <vnarsing@nvidia.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
…ers extraction - Remove orphan doc comment left on sandbox_list after print_yaml_line extraction - Change `pub mod commands` to `pub(crate) mod commands` in lib.rs - Move 6 unnecessarily pub-exported items to private use imports in run.rs - Restore stripped rationale comment in parse_cli_setting_value Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
258c301 to
f0b695a
Compare
|
The PR has been rebased with main, and the checks have been passing locally. |
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This behavior-preserving CLI refactor is project-valid under maintainer-scoped issue #2304 and its approved incremental command-module plan.
Head SHA: f0b695a8b87ad415766438f0df83940fa6976627
Thanks @varshaprasad96. I confirmed the rebased head is now mergeable and independently checked the complete diff after your update.
Review findings:
- No blocking findings remain. The moved implementations and error text are preserved, the existing
run::compatibility surface remains intact, and the credential/environment helpers do not change behavior.
Docs: Not needed because this is a structural refactor with no user-facing command, output, configuration, or workflow change.
Tests: Existing unit and integration coverage continues to exercise the preserved interfaces; review was code-only as required.
E2E: No test:* label is required for this behavior-preserving CLI-only move. The copy-pr validation gate still needs authorization for this head.
Next state: gator:watch-pipeline
|
/ok to test f0b695a |
|
@varshaprasad96 some checks need to be fixed then we can get ready to merge |
Summary
Extract ~950 lines of shared helper functions, types, and parsing utilities from the monolithic
run.rs(10,138 lines) into a newcommands/common.rsmodule. This is PR 1 of 6 in the incremental CLI refactor tracked by #2304.Related Issue
Part of #2304
Changes
commands/mod.rsandcommands/common.rsmodule scaffoldingphase_name,format_epoch_ms,format_elapsed,format_timestamp,print_yaml_line,print_sandbox_policy,short_hash,non_empty_or,format_timestamp_ms,format_optional_epoch_ms,truncate_status_field,truncate_display,format_setting_valueparse_env_pairs,parse_key_value_pairs,parse_secret_material_env_pairs,parse_credential_*,parse_cli_setting_value,parse_duration_to_msPolicyGetView,ProvisioningStep,ProvisioningDisplaymain.rsand integration tests viapub useinrun.rspub(crate) mod commandsto keep the module crate-internalTesting
cargo fmtandcargo clippypass with zero warnings--helpoutput byte-identical across all subcommands (gateway, sandbox, provider, service, settings, policy, inference)Checklist