fix: document required flags in examples and clean up error output#911
Open
clcollins wants to merge 6 commits into
Open
fix: document required flags in examples and clean up error output#911clcollins wants to merge 6 commits into
clcollins wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR standardizes CLI help text to ChangesCLI Help and Error Handling Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clcollins The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@cmd/alerts/list_alerts.go`:
- Around line 27-29: The example help text in cmd/alerts/list_alerts.go says
"firing alerts" but omits the --level flag; update the example command in the
command's help/Example string (the alerts list usage text) to include --level
firing (i.e., `osdctl alerts list --cluster-id ${CLUSTER_ID} --level firing
--reason "${REASON}"`) so the example matches the description.
In `@cmd/alerts/silence/add_silence.go`:
- Line 31: The command help text in the Long field contains a typo ("specfic")
and is awkwardly phrased; update the Long string (the Cobra command's Long
description) to correct the spelling to "specific" and improve readability, e.g.
"Add a new silence for a specific alert or for all alerts, including a comment
and duration." Replace the existing Long value with this clearer sentence in
add_silence.go.
In `@cmd/alerts/silence/clear_silence.go`:
- Line 27: The command's Long description (the Long field in clear_silence.go)
is unclear and has a typo "silenceid"; update it to a clear, user-facing
sentence such as "Expire all silences, or expire a specific silence by its
silence ID." Locate the Long string in clear_silence.go (the Long field of the
command definition) and replace the existing text with the improved phrasing.
In `@cmd/alerts/silence/silence_org.go`:
- Line 26: Update the user-facing help text in the org silence command's Long
description (the Long field in the command definition in silence_org.go) to fix
the typo "specfic" → "specific" and rephrase to "add a new silence for specific
alerts or all alerts with a comment and duration for an organization. OHSS
required for org-wide silence" so the sentence reads clearly and grammatically
correct.
In `@cmd/cluster/etcd_replace.go`:
- Line 46: Update the help text string that currently reads "Replaces an
unhealthy ectd node using the member id provided" to correct the typo "ectd" →
"etcd" (the Long field in the etcd replace command definition in
cmd/cluster/etcd_replace.go). Locate the Cobra command's Long description (the
string literal shown in the diff) and replace the misspelled word so the help
output matches the command name and examples.
In `@cmd/cluster/resize/infra_node.go`:
- Around line 75-78: Update the example invocations that pass the --ohss flag to
use the correct OHSS placeholder instead of the REASON placeholder: replace the
occurrences of "${REASON}" bound to --ohss with a dedicated "${OHSS}"
placeholder in the example command strings so --ohss reflects an OHSS ticket
(change both examples that include --ohss).
In `@main.go`:
- Around line 25-27: When handling the error returned from command.ExecuteC(),
guard against a nil resolved before calling resolved.CommandPath(): change the
error branch in main (where resolved, err := command.ExecuteC() is handled) to
first check if resolved != nil and use resolved.CommandPath(), otherwise print a
safe fallback usage string (e.g., "Run '<command> --help' for usage." or a
generic message) and then exit; ensure the nil-check surrounds any dereference
of resolved so no panic occurs.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a9fc1a7-48b6-4de7-b16e-1c67164821b6
📒 Files selected for processing (52)
cmd/account/rotate-secret.gocmd/alerts/list_alerts.gocmd/alerts/silence/add_silence.gocmd/alerts/silence/clear_silence.gocmd/alerts/silence/list_silence.gocmd/alerts/silence/silence_org.gocmd/cloudtrail/errors.gocmd/cloudtrail/permission-denied.gocmd/cluster/access/access.gocmd/cluster/access/cleanup.gocmd/cluster/cad/run.gocmd/cluster/changevolumetype.gocmd/cluster/checkbanneduser.gocmd/cluster/context.gocmd/cluster/detachstuckvolume.gocmd/cluster/etcd_health.gocmd/cluster/etcd_replace.gocmd/cluster/getenvvars.gocmd/cluster/health.gocmd/cluster/hypershift_info.gocmd/cluster/loggingcheck.gocmd/cluster/reports/create.gocmd/cluster/reports/get.gocmd/cluster/reports/list.gocmd/cluster/resize/controlplane_node.gocmd/cluster/resize/infra_node.gocmd/cluster/resize/requestserving_node.gocmd/cluster/snapshot.gocmd/cluster/ssh/key.gocmd/cluster/support/delete.gocmd/cluster/support/post.gocmd/cluster/support/status.gocmd/cluster/transferowner.gocmd/cluster/validatepullsecretext.gocmd/cluster/verifydns.gocmd/cmd.gocmd/cost/create.gocmd/cost/list.gocmd/cost/reconcile.gocmd/dynatrace/dashboardCmd.gocmd/dynatrace/urlCmd.gocmd/evidence/collect.gocmd/hcp/backup/backup.gocmd/hcp/get-cp-autoscaling-status/get_cp_autoscaling_status.gocmd/hcp/mustgather/mustGather.gocmd/hcp/status/status.gocmd/iampermissions/diff.gocmd/iampermissions/get.gocmd/iampermissions/save.gocmd/required_flags_test.gocmd/servicelog/list.gomain.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
main.go (1)
27-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fallback usage hint when
resolvedis nil.Lines 27-29 avoid the panic, but the nil path exits silently. Please emit a generic help hint in the
elsebranch so users always get guidance.Suggested patch
if err != nil { if resolved != nil { fmt.Fprintf(os.Stderr, "Run '%s --help' for usage.\n", resolved.CommandPath()) + } else { + fmt.Fprintln(os.Stderr, "Run 'osdctl --help' for usage.") } os.Exit(1) }🤖 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 `@main.go` around lines 27 - 29, The current code prints a usage hint only when resolved != nil; add an else branch to emit a fallback help hint when resolved is nil so users aren’t left without guidance. Modify the branch around resolved and CommandPath(): keep the existing fmt.Fprintf(os.Stderr, "Run '%s --help' for usage.\n", resolved.CommandPath()) for the non-nil case and in the else branch call fmt.Fprintf(os.Stderr, "Run '<command> --help' for usage.\n") or similar generic text (or derive the binary name from os.Args[0]) to ensure a helpful message is always printed.
🤖 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.
Duplicate comments:
In `@main.go`:
- Around line 27-29: The current code prints a usage hint only when resolved !=
nil; add an else branch to emit a fallback help hint when resolved is nil so
users aren’t left without guidance. Modify the branch around resolved and
CommandPath(): keep the existing fmt.Fprintf(os.Stderr, "Run '%s --help' for
usage.\n", resolved.CommandPath()) for the non-nil case and in the else branch
call fmt.Fprintf(os.Stderr, "Run '<command> --help' for usage.\n") or similar
generic text (or derive the binary name from os.Args[0]) to ensure a helpful
message is always printed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3019d305-3bf2-4dea-9206-26200709dcd9
📒 Files selected for processing (18)
cmd/account/cli.gocmd/alerts/list_alerts.gocmd/alerts/silence/add_silence.gocmd/alerts/silence/clear_silence.gocmd/alerts/silence/silence_org.gocmd/cluster/etcd_replace.gocmd/cluster/resize/infra_node.gocmd/evidence/collect.gocmd/promote/rhobs/rhobs.godocs/README.mddocs/osdctl_alert_list.mddocs/osdctl_alert_silence_add.mddocs/osdctl_alert_silence_expire.mddocs/osdctl_alert_silence_org.mddocs/osdctl_cluster_etcd-member-replace.mddocs/osdctl_cluster_resize_infra.mdmain.gopkg/utils/ocm_test.go
✅ Files skipped from review due to trivial changes (8)
- cmd/promote/rhobs/rhobs.go
- docs/osdctl_alert_silence_expire.md
- pkg/utils/ocm_test.go
- cmd/alerts/silence/clear_silence.go
- docs/osdctl_alert_silence_add.md
- cmd/alerts/silence/add_silence.go
- docs/README.md
- docs/osdctl_alert_silence_org.md
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/alerts/silence/silence_org.go
- docs/osdctl_cluster_resize_infra.md
- cmd/alerts/list_alerts.go
- cmd/cluster/resize/infra_node.go
- cmd/cluster/etcd_replace.go
- docs/osdctl_alert_list.md
- cmd/evidence/collect.go
- docs/osdctl_cluster_etcd-member-replace.md
Member
Author
|
/label tide/merge-method-squash 🤖 Claude claude@anthropic.com commenting on behalf of @clcollins |
openshift-ci
Bot
added
the
tide/merge-method-squash
MateSaary
reviewed
Jun 8, 2026
| if err := command.Execute(); err != nil { | ||
| _, err := fmt.Fprintf(os.Stderr, "%v\n", err) | ||
| if err != nil { | ||
| fmt.Println("Error while printing to stderr: ", err.Error()) |
Member
There was a problem hiding this comment.
It looks like this print causes an incorrect command to duplicate the error message printed by cobra already, i.e.
$ osdctl test
Error: unknown command "test" for "osdctl"
Did you mean this?
cost
Run 'osdctl --help' for usage.
Run 'osdctl --help' for usage.
exit status 1
The removal of printing os.Stderr here also means that some commands (seems like mostly the rhobs and org ones) which have a SilenceErrors applied won't print an error message at all;
$ osdctl rhobs metrics "up" -o badformat
Run 'osdctl rhobs metrics --help' for usage.
I guess the cleanest solution would be to bring those commands in-line with the rest of the repo 🙂
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/rhobs/metricsCmd.go (1)
16-20: 💤 Low valueConsider removing redundant argument validation.
The
Args: cobra.ExactArgs(1)validator on line 16 ensures exactly one argument is provided beforeRunEexecutes, making the manuallen(args) != 1check on lines 18-20 unreachable. This check can be safely removed.♻️ Simplify by removing dead code
RunE: func(cmd *cobra.Command, args []string) error { - if len(args) != 1 { - return fmt.Errorf("exactly one Prometheus expression must be provided as an argument") - } - outputFormat, err := GetMetricsFormatFromString(outputFormatStr)🤖 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 `@cmd/rhobs/metricsCmd.go` around lines 16 - 20, Remove the redundant runtime argument check inside the RunE handler: the CLI command already sets Args: cobra.ExactArgs(1), so the manual conditional that tests len(args) != 1 and returns an error should be deleted; update the RunE anonymous function (the handler defined as RunE) to assume a single arg is present and remove the unreachable len(args) check to simplify the code.
🤖 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.
Nitpick comments:
In `@cmd/rhobs/metricsCmd.go`:
- Around line 16-20: Remove the redundant runtime argument check inside the RunE
handler: the CLI command already sets Args: cobra.ExactArgs(1), so the manual
conditional that tests len(args) != 1 and returns an error should be deleted;
update the RunE anonymous function (the handler defined as RunE) to assume a
single arg is present and remove the unreachable len(args) check to simplify the
code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20a79d7d-94d9-4426-9d73-2d510f582f02
📒 Files selected for processing (41)
cmd/account/rotate-secret.gocmd/alerts/list_alerts.gocmd/alerts/silence/add_silence.gocmd/alerts/silence/clear_silence.gocmd/alerts/silence/list_silence.gocmd/alerts/silence/silence_org.gocmd/cluster/access/access.gocmd/cluster/access/cleanup.gocmd/cluster/checkbanneduser.gocmd/cluster/context.gocmd/cluster/detachstuckvolume.gocmd/cluster/etcd_health.gocmd/cluster/etcd_replace.gocmd/cluster/getenvvars.gocmd/cluster/health.gocmd/cluster/loggingcheck.gocmd/cluster/support/delete.gocmd/cluster/support/status.gocmd/cluster/transferowner.gocmd/cluster/verifydns.gocmd/dynatrace/dashboardCmd.gocmd/dynatrace/urlCmd.gocmd/iampermissions/diff.gocmd/iampermissions/get.gocmd/iampermissions/save.gocmd/org/aws-accounts.gocmd/org/clusters.gocmd/org/current.gocmd/org/customers.gocmd/org/describe.gocmd/org/get.gocmd/org/labels.gocmd/org/users.gocmd/rhobs/cellCmd.gocmd/rhobs/logsCmd.gocmd/rhobs/mcpCmd.gocmd/rhobs/metricsCmd.gocmd/rhobs/rootCmd.gocmd/servicelog/list.gocmd/upgrade.gomain.go
✅ Files skipped from review due to trivial changes (19)
- cmd/cluster/verifydns.go
- cmd/rhobs/mcpCmd.go
- cmd/dynatrace/dashboardCmd.go
- cmd/cluster/etcd_health.go
- cmd/dynatrace/urlCmd.go
- cmd/iampermissions/get.go
- cmd/cluster/detachstuckvolume.go
- cmd/alerts/silence/silence_org.go
- cmd/iampermissions/diff.go
- cmd/alerts/list_alerts.go
- cmd/alerts/silence/add_silence.go
- cmd/cluster/checkbanneduser.go
- cmd/cluster/support/delete.go
- cmd/alerts/silence/list_silence.go
- cmd/cluster/transferowner.go
- cmd/cluster/context.go
- cmd/servicelog/list.go
- cmd/cluster/loggingcheck.go
- cmd/cluster/access/access.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/cluster/access/cleanup.go
- main.go
- cmd/account/rotate-secret.go
- cmd/iampermissions/save.go
- cmd/cluster/support/status.go
- cmd/alerts/silence/clear_silence.go
Required flags like --reason and --cluster-id were missing from command
examples, causing users to copy-paste incomplete invocations and hit
confusing errors. Additionally, missing-flag errors were buried under
a wall of usage text, and printed twice (once by cobra, once by main).
- Add SilenceUsage in PersistentPreRun so missing-flag and RunE errors
no longer dump full usage text (unknown commands/flags still show it)
- Replace duplicate error print in main.go with a help hint
- Add Example fields to 37 commands that had required flags but no examples
- Move examples from Long field to Example field where misplaced
- Fix incorrect shorthand (-c vs -C) in resize control-plane example
- Standardize all placeholders to ${VARIABLE} style (no hard-coded
ticket numbers or angle-bracket placeholders in examples)
- Add regression test that walks the command tree and validates all
required flags appear in their command's Example field
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CodeRabbit review fixes:
- Add --level firing example to alerts list
- Fix typos in Long descriptions: "specfic" -> "specific", "ectd" -> "etcd"
- Use ${OHSS} placeholder for --ohss flag instead of ${REASON}
- Guard against nil resolved command in main.go error path
Pre-existing lint fixes:
- evidence/collect.go: replace WriteString(Sprintf(...)) with Fprintf (staticcheck QF1012)
- account/cli.go: suppress gosec G117 false positive on credential output
- promote/rhobs/rhobs.go: suppress gosec G703 false positive on hardcoded paths
- pkg/utils/ocm_test.go: suppress gosec G117 false positive on test fixtures
Regenerate docs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address Mate's review: the PR 911 main.go change caused duplicate "Run ... --help" hints for unknown commands and swallowed errors entirely for commands with SilenceErrors: true (rhobs, org, upgrade). - Fix main.go to print hint only when SilenceUsage is set (i.e. PersistentPreRun ran on a valid command), avoiding duplication - Remove SilenceErrors: true from all 15 commands — none had a documented reason and it was boilerplate from initial implementations - Keep SilenceUsage: true on mcp server/config for MCP protocol safety Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
clcollins
force-pushed
the
worktree-snappy-jingling-elephant
branch
from
4553bb3 to
3b0cf32
Compare
openshift-ci
Bot
removed
the
needs-rebase
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cmd/iampermissions/save.go (1)
49-50: ⚡ Quick winConsider using placeholder variables for consistency.
The example uses literal values
/tmp/policiesand4.15.0instead of placeholder variables like${DIR}and${RELEASE_VERSION}. Other commands in this PR consistently use the${VARIABLE}placeholder style (e.g.,${CLUSTER_ID},${OU_ID},${REASON}).♻️ Suggested alignment with placeholder style
- # Save IAM permissions to a directory - osdctl iampermissions save --dir /tmp/policies --release-version 4.15.0`, + # Save IAM permissions to a directory + osdctl iampermissions save --dir ${DIR} --release-version ${RELEASE_VERSION}`,🤖 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 `@cmd/iampermissions/save.go` around lines 49 - 50, The example in the help text uses hard-coded values; update the example string in cmd/iampermissions/save.go to use placeholder variables for consistency (replace `/tmp/policies` with `${DIR}` and `4.15.0` with `${RELEASE_VERSION}`), preserving the existing formatting and backticks around the full command so it matches other examples that use `${VARIABLE}` style.cmd/account/cli.go (1)
109-109: 💤 Low valueSuppression is appropriate; consider clarifying the comment.
The
//nolint:gosecsuppression is correct for this use case—the command's purpose is to output temporary AWS credentials to stdout for CLI consumption. However, the phrase "G117 false positive" is slightly misleading: gosec correctly detected credential output; this is intentional behavior rather than an incorrect detection (false positive).📝 Optional: rephrase for accuracy
- if err := enc.Encode(out); err != nil { //nolint:gosec // G117 false positive — intentionally outputting AWS credentials + if err := enc.Encode(out); err != nil { //nolint:gosec // G117 - intentional: CLI outputs temporary credentials to stdout🤖 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 `@cmd/account/cli.go` at line 109, Update the inline lint suppression on the enc.Encode(out) call to clearly state that the G117 gosec finding is expected and intentional because this command purposefully outputs temporary AWS credentials for CLI consumption; replace "G117 false positive" with a concise justification like "intentionally outputting temporary AWS credentials for CLI use — safe/expected" and keep the existing //nolint:gosec annotation on enc.Encode to avoid silencing other checks.
🤖 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 `@docs/osdctl_alert_list.md`:
- Around line 15-24: The examples in the snippet use the plural command "osdctl
alerts list" while the page title/synopsis use "osdctl alert list"; update all
example lines to use the singular "osdctl alert list" so they match the
synopsis/title and prevent broken copy/paste, i.e., replace each occurrence of
"osdctl alerts list" in the shown examples with "osdctl alert list".
In `@docs/osdctl_alert_silence_add.md`:
- Around line 15-24: Update the example commands to use the documented command
path "osdctl alert silence add" instead of "osdctl alerts silence add"; locate
the examples shown (three occurrences of the example command) and replace each
"osdctl alerts silence add" with "osdctl alert silence add" so the examples
(including the flags --cluster-id, --alertname, --all, --duration, --comment,
--reason) match the documented command path exactly.
In `@docs/osdctl_servicelog_list.md`:
- Around line 9-11: Update the synopsis placeholder to use the same style as the
examples: replace the `<cluster-identifier>` placeholder in the command string
`osdctl servicelog list --cluster-id <cluster-identifier> [flags] [options]`
with `${CLUSTER_ID}` so the synopsis matches the examples and avoids mixed
placeholder formats.
---
Nitpick comments:
In `@cmd/account/cli.go`:
- Line 109: Update the inline lint suppression on the enc.Encode(out) call to
clearly state that the G117 gosec finding is expected and intentional because
this command purposefully outputs temporary AWS credentials for CLI consumption;
replace "G117 false positive" with a concise justification like "intentionally
outputting temporary AWS credentials for CLI use — safe/expected" and keep the
existing //nolint:gosec annotation on enc.Encode to avoid silencing other
checks.
In `@cmd/iampermissions/save.go`:
- Around line 49-50: The example in the help text uses hard-coded values; update
the example string in cmd/iampermissions/save.go to use placeholder variables
for consistency (replace `/tmp/policies` with `${DIR}` and `4.15.0` with
`${RELEASE_VERSION}`), preserving the existing formatting and backticks around
the full command so it matches other examples that use `${VARIABLE}` style.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9f70ed1-c9a5-4e61-bbf1-2d14f6dd6cbe
📒 Files selected for processing (117)
cmd/account/cli.gocmd/account/rotate-secret.gocmd/alerts/list_alerts.gocmd/alerts/silence/add_silence.gocmd/alerts/silence/clear_silence.gocmd/alerts/silence/list_silence.gocmd/alerts/silence/silence_org.gocmd/cloudtrail/errors.gocmd/cloudtrail/permission-denied.gocmd/cluster/access/access.gocmd/cluster/access/cleanup.gocmd/cluster/cad/run.gocmd/cluster/changevolumetype.gocmd/cluster/checkbanneduser.gocmd/cluster/context.gocmd/cluster/detachstuckvolume.gocmd/cluster/etcd_health.gocmd/cluster/etcd_replace.gocmd/cluster/getenvvars.gocmd/cluster/health.gocmd/cluster/hypershift_info.gocmd/cluster/loggingcheck.gocmd/cluster/reports/create.gocmd/cluster/reports/get.gocmd/cluster/reports/list.gocmd/cluster/resize/controlplane_node.gocmd/cluster/resize/infra_node.gocmd/cluster/resize/requestserving_node.gocmd/cluster/snapshot.gocmd/cluster/ssh/key.gocmd/cluster/support/delete.gocmd/cluster/support/post.gocmd/cluster/support/status.gocmd/cluster/transferowner.gocmd/cluster/validatepullsecretext.gocmd/cluster/verifydns.gocmd/cmd.gocmd/cost/create.gocmd/cost/list.gocmd/cost/reconcile.gocmd/dynatrace/dashboardCmd.gocmd/dynatrace/urlCmd.gocmd/evidence/collect.gocmd/hcp/backup/backup.gocmd/hcp/get-cp-autoscaling-status/get_cp_autoscaling_status.gocmd/hcp/mustgather/mustGather.gocmd/hcp/status/status.gocmd/iampermissions/diff.gocmd/iampermissions/get.gocmd/iampermissions/save.gocmd/org/aws-accounts.gocmd/org/clusters.gocmd/org/current.gocmd/org/customers.gocmd/org/describe.gocmd/org/get.gocmd/org/labels.gocmd/org/users.gocmd/promote/rhobs/rhobs.gocmd/required_flags_test.gocmd/rhobs/cellCmd.gocmd/rhobs/logsCmd.gocmd/rhobs/mcpCmd.gocmd/rhobs/metricsCmd.gocmd/rhobs/rootCmd.gocmd/servicelog/list.gocmd/upgrade.godocs/README.mddocs/osdctl_account_rotate-secret.mddocs/osdctl_alert_list.mddocs/osdctl_alert_silence_add.mddocs/osdctl_alert_silence_expire.mddocs/osdctl_alert_silence_list.mddocs/osdctl_alert_silence_org.mddocs/osdctl_cloudtrail_errors.mddocs/osdctl_cloudtrail_permission-denied-events.mddocs/osdctl_cluster_break-glass.mddocs/osdctl_cluster_break-glass_cleanup.mddocs/osdctl_cluster_cad_run.mddocs/osdctl_cluster_change-ebs-volume-type.mddocs/osdctl_cluster_check-banned-user.mddocs/osdctl_cluster_context.mddocs/osdctl_cluster_detach-stuck-volume.mddocs/osdctl_cluster_etcd-health-check.mddocs/osdctl_cluster_etcd-member-replace.mddocs/osdctl_cluster_get-env-vars.mddocs/osdctl_cluster_health.mddocs/osdctl_cluster_hypershift-info.mddocs/osdctl_cluster_logging-check.mddocs/osdctl_cluster_reports_create.mddocs/osdctl_cluster_reports_get.mddocs/osdctl_cluster_reports_list.mddocs/osdctl_cluster_resize_control-plane.mddocs/osdctl_cluster_resize_infra.mddocs/osdctl_cluster_resize_request-serving-nodes.mddocs/osdctl_cluster_snapshot.mddocs/osdctl_cluster_ssh_key.mddocs/osdctl_cluster_support_delete.mddocs/osdctl_cluster_support_post.mddocs/osdctl_cluster_support_status.mddocs/osdctl_cluster_transfer-owner.mddocs/osdctl_cluster_validate-pull-secret-ext.mddocs/osdctl_cluster_verify-dns.mddocs/osdctl_cost_create.mddocs/osdctl_cost_list.mddocs/osdctl_cost_reconcile.mddocs/osdctl_evidence_collect.mddocs/osdctl_hcp_backup.mddocs/osdctl_hcp_get-cp-autoscaling-status.mddocs/osdctl_hcp_must-gather.mddocs/osdctl_hcp_status.mddocs/osdctl_iampermissions_diff.mddocs/osdctl_iampermissions_get.mddocs/osdctl_iampermissions_save.mddocs/osdctl_servicelog_list.mdmain.gopkg/utils/ocm_test.go
✅ Files skipped from review due to trivial changes (40)
- cmd/alerts/silence/list_silence.go
- cmd/cluster/etcd_replace.go
- cmd/cluster/loggingcheck.go
- docs/osdctl_hcp_get-cp-autoscaling-status.md
- cmd/cloudtrail/errors.go
- cmd/cluster/verifydns.go
- docs/osdctl_cloudtrail_errors.md
- docs/osdctl_cluster_validate-pull-secret-ext.md
- docs/osdctl_cluster_resize_control-plane.md
- cmd/cluster/support/status.go
- docs/osdctl_cluster_snapshot.md
- cmd/iampermissions/diff.go
- cmd/dynatrace/urlCmd.go
- cmd/cluster/context.go
- docs/osdctl_cluster_change-ebs-volume-type.md
- cmd/cluster/detachstuckvolume.go
- docs/osdctl_cluster_resize_infra.md
- docs/osdctl_evidence_collect.md
- cmd/cluster/resize/controlplane_node.go
- cmd/cluster/checkbanneduser.go
- cmd/cluster/access/access.go
- cmd/cluster/reports/get.go
- cmd/iampermissions/get.go
- docs/osdctl_cluster_ssh_key.md
- cmd/org/users.go
- cmd/cluster/ssh/key.go
- docs/osdctl_hcp_must-gather.md
- cmd/cluster/reports/create.go
- pkg/utils/ocm_test.go
- cmd/cluster/health.go
- cmd/cluster/support/delete.go
- cmd/hcp/get-cp-autoscaling-status/get_cp_autoscaling_status.go
- cmd/cluster/resize/infra_node.go
- cmd/cluster/etcd_health.go
- docs/osdctl_hcp_status.md
- cmd/promote/rhobs/rhobs.go
- cmd/alerts/silence/clear_silence.go
- cmd/cost/create.go
- cmd/cluster/resize/requestserving_node.go
- cmd/dynatrace/dashboardCmd.go
🚧 Files skipped from review as they are similar to previous changes (32)
- cmd/hcp/mustgather/mustGather.go
- cmd/cloudtrail/permission-denied.go
- cmd/cluster/snapshot.go
- cmd/org/clusters.go
- cmd/cluster/reports/list.go
- cmd/alerts/silence/add_silence.go
- cmd/account/rotate-secret.go
- cmd/hcp/backup/backup.go
- docs/osdctl_cluster_resize_request-serving-nodes.md
- cmd/rhobs/mcpCmd.go
- cmd/cluster/access/cleanup.go
- cmd/cluster/changevolumetype.go
- cmd/rhobs/logsCmd.go
- cmd/hcp/status/status.go
- cmd/cluster/cad/run.go
- cmd/servicelog/list.go
- cmd/alerts/list_alerts.go
- docs/osdctl_hcp_backup.md
- docs/osdctl_cluster_support_post.md
- cmd/org/describe.go
- main.go
- cmd/alerts/silence/silence_org.go
- cmd/org/aws-accounts.go
- cmd/rhobs/metricsCmd.go
- cmd/org/current.go
- cmd/cluster/hypershift_info.go
- cmd/rhobs/cellCmd.go
- cmd/cmd.go
- cmd/evidence/collect.go
- cmd/org/labels.go
- cmd/cluster/validatepullsecretext.go
- cmd/required_flags_test.go
Comment on lines
+15
to
+24
| ``` | ||
| # List all alerts for a cluster | ||
| osdctl alerts list --cluster-id ${CLUSTER_ID} --reason "${REASON}" | ||
|
|
||
| # List only firing alerts | ||
| osdctl alerts list --cluster-id ${CLUSTER_ID} --level firing --reason "${REASON}" | ||
|
|
||
| # List only critical alerts | ||
| osdctl alerts list --cluster-id ${CLUSTER_ID} --level critical --reason "${REASON}" | ||
| ``` |
There was a problem hiding this comment.
Fix command name mismatch in examples (alerts vs alert).
Examples use osdctl alerts list, but this page’s synopsis and title use osdctl alert list. Keep them consistent to avoid broken copy/paste usage.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/osdctl_alert_list.md` around lines 15 - 24, The examples in the snippet
use the plural command "osdctl alerts list" while the page title/synopsis use
"osdctl alert list"; update all example lines to use the singular "osdctl alert
list" so they match the synopsis/title and prevent broken copy/paste, i.e.,
replace each occurrence of "osdctl alerts list" in the shown examples with
"osdctl alert list".
Comment on lines
+15
to
+24
| ``` | ||
| # Silence a specific alert | ||
| osdctl alerts silence add --cluster-id ${CLUSTER_ID} --alertname "KubePodNotReady" --reason "${REASON}" | ||
|
|
||
| # Silence all alerts for a cluster | ||
| osdctl alerts silence add --cluster-id ${CLUSTER_ID} --all --reason "${REASON}" | ||
|
|
||
| # Silence an alert with custom duration and comment | ||
| osdctl alerts silence add --cluster-id ${CLUSTER_ID} --alertname "KubePodNotReady" --duration 2h --comment "Investigating pod issue" --reason "${REASON}" | ||
| ``` |
There was a problem hiding this comment.
Correct example command path to match documented command.
Examples use osdctl alerts silence add, while this command page documents osdctl alert silence add. Align the examples with the documented command path.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/osdctl_alert_silence_add.md` around lines 15 - 24, Update the example
commands to use the documented command path "osdctl alert silence add" instead
of "osdctl alerts silence add"; locate the examples shown (three occurrences of
the example command) and replace each "osdctl alerts silence add" with "osdctl
alert silence add" so the examples (including the flags --cluster-id,
--alertname, --all, --duration, --comment, --reason) match the documented
command path exactly.
Comment on lines
+9
to
+11
| ``` | ||
| osdctl servicelog list --cluster-id <cluster-identifier> [flags] [options] | ||
| ``` |
There was a problem hiding this comment.
Unify synopsis placeholder style with examples.
Line 10 uses <cluster-identifier> while Lines 17/20/23 use ${CLUSTER_ID}. Keep one style (prefer ${CLUSTER_ID}) to avoid conflicting docs patterns.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/osdctl_servicelog_list.md` around lines 9 - 11, Update the synopsis
placeholder to use the same style as the examples: replace the
`<cluster-identifier>` placeholder in the command string `osdctl servicelog list
--cluster-id <cluster-identifier> [flags] [options]` with `${CLUSTER_ID}` so the
synopsis matches the examples and avoids mixed placeholder formats.
Contributor
|
@clcollins: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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
2 participants
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.
Summary
SilenceUsageinPersistentPreRunso missing-flag andRunEerrors show only the error + a help hint, not a wall of usage text. Unknown commands/flags still show usage.main.goto useExecuteC()and replaced the redundant error print withRun '<command> --help' for usage.Examplefields on 37 commands that had required flags (e.g.--reason,--cluster-id) but no examples, or had examples that omitted required flags.OHSS-1234,SREP-3811,OSD-XYZ) and angle-bracket placeholders (<cluster-id>) with${VARIABLE}style across all examples.TestRequiredFlagsDocumentedInExampleswalks the entire command tree and validates everyMarkFlagRequiredflag appears in its command'sExamplefield.Before / After
Secondary Lint Fixes
Pre-existing lint issues fixed to get
make lintpassing clean:cmd/evidence/collect.go: replacedWriteString(fmt.Sprintf(...))withfmt.Fprintf(...)(staticcheck QF1012, 15 instances)cmd/account/cli.go: suppressed gosec G117 false positive on intentional credential outputcmd/promote/rhobs/rhobs.go: suppressed gosec G703 false positive on hardcoded path candidates (2 instances)pkg/utils/ocm_test.go: suppressed gosec G117 false positive on test fixtures with fake credentials (3 instances)Test plan
go build ./...compilesgo test ./cmd/...— all existing tests passmake lint— 0 issuesmake verify-docs— docs regenerated and up to dateTestRequiredFlagsDocumentedInExamplespasses--help— confirm full help text still appears🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Tests
Improvements