Conversation
| Arg::new("environment") | ||
| .short('e') | ||
| .long("environment") | ||
| .default_value("default") |
There was a problem hiding this comment.
I'm not sure this is the right thing to do here. I guess that's what default is for?
There was a problem hiding this comment.
pretty sure yeah, looks like we have it everywhere we have this argument
|
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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review this 💩 ! |
|
✅ Actions performedReview triggered.
|
| "Name".bold().yellow().to_string(), | ||
| "Type".bold().yellow().to_string(), | ||
| "Environment".bold().yellow().to_string(), |
There was a problem hiding this comment.
i know there's a couple other places doing this, but it really shouldn't be necessary, we already format headers yellow in the output::table
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the output module
fd 'output.rs' --type f
# Search for the table function definition
rg -n 'fn table' --type rust -A 30
Length of output: 2376
🏁 Script executed:
#!/bin/bash
# Check how other commands pass headers to output::table
rg -n 'bold\(\)\.yellow\(\)\|yellow\(\)\.bold\(\)\|\.yellow()\|headers.*vec' --type rust -B2 -A2 crates/tower-cmd/src/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how other commands pass headers to output::table
rg -n 'bold\(\)' --type rust crates/tower-cmd/src/ -A1 -B1
echo "---"
# Also check how other commands build header vecs
rg -n '"Name"\|"Type"\|"Environment"\|headers = vec' --type rust crates/tower-cmd/src/
Length of output: 2950
@konstantinoscs — socksy's claim is partially correct. Here's what the code actually does:
In crates/tower-cmd/src/output.rs (Line 461), output::table applies .yellow() to headers internally:
.title(headers.iter().map(|h| h.yellow().to_string()));So passing headers that are already .bold().yellow() means .yellow() is applied twice — which is redundant. However, output::table does not apply .bold(), so the bold styling only comes from the callers (catalogs.rs, secrets.rs).
This means:
- If you pass plain strings → headers are yellow only (from
output::table) - If you pass
.bold().yellow()strings → headers are bold + yellow (yellow is double-applied but has no extra effect)
The cleanest fix would be one of these two options:
Option A — Strip .yellow() from callers (keep .bold()), since output::table adds it:
let headers = vec![
"Name".bold().to_string(),
"Type".bold().to_string(),
"Environment".bold().to_string(),
];Option B — Update output::table to apply .bold().yellow() so callers can pass plain strings:
// In output.rs
.title(headers.iter().map(|h| h.bold().yellow().to_string()));Option B would also fix the same redundancy in secrets.rs at the source, which would be the more principled fix.
Either way, this is an existing pattern from secrets.rs that predates this PR, so the concern applies equally to both files.
There was a problem hiding this comment.
@coderabbitai and @socksy: I've consolidated all headers to use the same style. PTAL
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check current state of headers in catalogs.rs, secrets.rs, and output.rs table function
echo "=== catalogs.rs headers ==="
rg -n 'bold\(\)|yellow\(\)|headers\s*=\s*vec' --type rust crates/tower-cmd/src/catalogs.rs -A1 -B1
echo "=== secrets.rs headers ==="
rg -n 'bold\(\)|yellow\(\)|headers\s*=\s*vec' --type rust crates/tower-cmd/src/secrets.rs -A1 -B1
echo "=== output.rs table title ==="
rg -n 'title\|bold\(\)|yellow\(\)' --type rust crates/tower-cmd/src/output.rs -A2 -B2
Length of output: 1774
@konstantinoscs — The fix looks great! Option B was applied correctly:
catalogs.rsandsecrets.rsnow pass plain strings (e.g.,vec!["Name", "Type", "Environment"]) tooutput::tableoutput.rs(Line 461) centralizes the styling:.title(headers.iter().map(|h| h.bold().yellow().to_string()))
This is the cleaner, more principled approach — styling is defined in one place and callers don't need to worry about it. LGTM! ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/tower-cmd/src/catalogs.rs (1)
79-121: Consider usingwith_spinnerfor consistency withdo_listand sibling modules.The
do_showfunction manually calls the API and handles errors, whiledo_listusesoutput::with_spinner. Based on the patterns inteams.rs(line 59) andapps.rs(line 206), usingwith_spinnerprovides a consistent UX with a progress indicator during API calls.♻️ Suggested refactor using with_spinner
pub async fn do_show(config: Config, args: &ArgMatches) { let name = args .get_one::<String>("catalog_name") .expect("catalog_name is required"); let env = cmd::get_string_flag(args, "environment"); - match api::describe_catalog(&config, name, &env).await { - Ok(response) => { - if output::get_output_mode().is_json() { - output::json(&response); - return; - } - - let catalog = &response.catalog; - - output::detail("Catalog", &catalog.name); - output::detail("Type", &catalog.r#type); - output::detail("Environment", &catalog.environment); - - if !catalog.properties.is_empty() { - output::newline(); - output::header("Properties"); - - let headers = vec![ - "Name".bold().yellow().to_string(), - "Preview".bold().yellow().to_string(), - ]; - let data = catalog - .properties - .iter() - .map(|prop| { - vec![ - prop.name.clone(), - prop.preview.dimmed().to_string(), - ] - }) - .collect(); - output::table(headers, data, Some(&response)); - } - } - Err(err) => output::tower_error_and_die(err, "Fetching catalog details failed"), + let response = + output::with_spinner("Fetching catalog details", api::describe_catalog(&config, name, &env)).await; + + if output::get_output_mode().is_json() { + output::json(&response); + return; } + + let catalog = &response.catalog; + + output::detail("Catalog", &catalog.name); + output::detail("Type", &catalog.r#type); + output::detail("Environment", &catalog.environment); + + if !catalog.properties.is_empty() { + output::newline(); + output::header("Properties"); + + let headers = vec![ + "Name".bold().yellow().to_string(), + "Preview".bold().yellow().to_string(), + ]; + let data = catalog + .properties + .iter() + .map(|prop| { + vec![ + prop.name.clone(), + prop.preview.dimmed().to_string(), + ] + }) + .collect(); + output::table(headers, data, Some(&response)); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tower-cmd/src/catalogs.rs` around lines 79 - 121, Replace the manual API call and match in do_show with output::with_spinner that executes api::describe_catalog(&config, name, &env).await to keep parity with do_list and sibling modules; move the Ok(response) success path (JSON branch, extracting response.catalog, calling output::detail, output::newline/header/table) into the spinner's success continuation, and ensure any error returned from with_spinner is forwarded to output::tower_error_and_die(err, "Fetching catalog details failed") so error handling remains identical. Use the same response variable names and preserve building headers/data for output::table exactly as currently implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/tower-cmd/src/catalogs.rs`:
- Around line 79-121: Replace the manual API call and match in do_show with
output::with_spinner that executes api::describe_catalog(&config, name,
&env).await to keep parity with do_list and sibling modules; move the
Ok(response) success path (JSON branch, extracting response.catalog, calling
output::detail, output::newline/header/table) into the spinner's success
continuation, and ensure any error returned from with_spinner is forwarded to
output::tower_error_and_die(err, "Fetching catalog details failed") so error
handling remains identical. Use the same response variable names and preserve
building headers/data for output::table exactly as currently implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dad485d-31bb-4293-a421-181e90c3fe6a
📒 Files selected for processing (3)
crates/tower-cmd/src/api.rscrates/tower-cmd/src/catalogs.rscrates/tower-cmd/src/lib.rs
konstantinoscs
force-pushed
the
feature/tow-1760-introduce-catalog-secrets-commands
branch
from
17376aa to
0de2e26
Compare
konstantinoscs
force-pushed
the
feature/tow-1760-introduce-catalog-secrets-commands
branch
from
0de2e26 to
6323464
Compare
3 participants
Adding catalog commands to the cli.
How they will look:
Closes TOW-1760
Summary by CodeRabbit
Release Notes
catalogscommand with subcommands to list and view catalog detailslistsubcommand displays catalogs with Name, Type, and Environment columns; supports filtering by environment and viewing all available optionsshowsubcommand provides detailed catalog information including properties; supports JSON output format