OTA-1814: docs(alerts): README updates for #2279#2296
Conversation
…d infra. add more context Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
|
@nbottari9: This pull request references OTA-1814 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR restructures the ChangesRecommend examples documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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.
Actionable comments posted: 2
🤖 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 `@pkg/cli/admin/upgrade/recommend/examples/README.md`:
- Around line 22-25: The update instructions reference the wrong fixture name;
the examples in examples_test.go derive output files from the testcase name, not
TESTCASE.out. Update the README text to match the existing fixture naming
contract used by examples_test.go and the related example outputs (for example,
the *.output, *.show-outdated-releases-output, and *.version-<VERSION>-output
patterns) so the documented UPDATE workflow points to the correct generated
files.
- Line 16: The README example uses the wrong CLI flag for the upgrade recommend
command. Update the example in the recommend examples README to reference the
`--version` flag exposed by `recommend.go` instead of `--to`, so the documented
command matches the actual `oc adm upgrade recommend` interface and the
`TESTCASE.version-<VERSION>-output` description stays aligned.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3738f3cc-4c02-4c31-b88d-18782c5b492b
📒 Files selected for processing (1)
pkg/cli/admin/upgrade/recommend/examples/README.md
| --- | ||
| * `TESTCASE.output`: expected output of `oc adm upgrade recommend`. | ||
| * `TESTCASE.show-outdated-releases-output`: expected output of `oc adm upgrade recommend --show-outdated-releases`. | ||
| * `TESTCASE.version-<VERSION>-output`: expected output of `oc adm upgrade recommend --to <VERSION>`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use --version, not --to.
The command contract in recommend.go exposes --version; --to is not a supported flag, so this example will send readers to the wrong CLI surface.
🤖 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 `@pkg/cli/admin/upgrade/recommend/examples/README.md` at line 16, The README
example uses the wrong CLI flag for the upgrade recommend command. Update the
example in the recommend examples README to reference the `--version` flag
exposed by `recommend.go` instead of `--to`, so the documented command matches
the actual `oc adm upgrade recommend` interface and the
`TESTCASE.version-<VERSION>-output` description stays aligned.
| * When the testcase is executed with a non-empty `UPDATE` environmental variable, it will update the `TESTCASE.out` fixture: | ||
| ```console | ||
| $ UPDATE=yes go test -v ./pkg/cli/admin/upgrade/recommend/... | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the fixture name in the update instructions.
examples_test.go derives output fixtures from *-cv.yaml, and the outputs in this README are named *.output / *.show-outdated-releases-output / *.version-<VERSION>-output; TESTCASE.out does not match that contract.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 24-24: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 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 `@pkg/cli/admin/upgrade/recommend/examples/README.md` around lines 22 - 25, The
update instructions reference the wrong fixture name; the examples in
examples_test.go derive output files from the testcase name, not TESTCASE.out.
Update the README text to match the existing fixture naming contract used by
examples_test.go and the related example outputs (for example, the *.output,
*.show-outdated-releases-output, and *.version-<VERSION>-output patterns) so the
documented UPDATE workflow points to the correct generated files.
| * `TESTCASE-cv.yaml`: ClusterVersion object (required, created by `oc get clusterversion version -o yaml`). Lists are also supported. | ||
| * `TESTCASE-featuregate.yaml`: FeatureGate object (optional, created by `oc get featuregate cluster -o yaml`). Lists are NOT supported. | ||
| * `TESTCASE-infrastructure.yaml`: Infrastructure object (optional, created by `oc get infrastructure cluster -o yaml`). Lists are NOT supported | ||
| * `TESTCASE-alerts.json`: Alerts currently present in the cluster (optional, created by `OC_ENABLE_CMD_INSPECT_ALERTS=true oc adm inspect-alerts`) |
There was a problem hiding this comment.
Interesting, Trevor did not use this command directly in 26ec958
| * `TESTCASE-alerts.json`: Alerts currently present in the cluster (optional, created by `OC_ENABLE_CMD_INSPECT_ALERTS=true oc adm inspect-alerts`) | |
| * `TESTCASE-alerts.json`: Running alerts in the cluster (optional, expected output of `OC_ENABLE_CMD_INSPECT_ALERTS=true oc adm inspect-alerts`) |
The wording is from
$ OC_ENABLE_CMD_INSPECT_ALERTS=true oc adm inspect-alerts -h
Collect information about running alerts.
Usage:
oc adm inspect-alerts [flags] [options]
Use "oc options" for a list of global command-line options (applies to all commands).There was a problem hiding this comment.
What change should be made here?
Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
|
/lgtm /label tide/merge-method-squash |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, nbottari9 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 |
|
/retest |
|
https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-openshift-oc-main-okd-scos-images /test okd-scos-images |
|
/verified bypass |
|
@nbottari9: The DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test images |
|
@nbottari9: 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. |
README update for #2279
alerts.json-featuregate.yamland-infrastructure.yamlSummary by CodeRabbit