OCM-24429 | feat: Updated rosa list ocm-role to display console access#3250
OCM-24429 | feat: Updated rosa list ocm-role to display console access#3250robpblake wants to merge 1 commit 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:
📝 WalkthroughWalkthroughThis PR adds a NoConsole role tag key and a NoConsole field on aws.Role with RoleYes/RoleNo constants, updates ListOCMRoles to read the no_console_role tag (forcing Admin=No when set) and uses the constants in sorting, refactors the rosa list ocm-roles rendering into printOCMRoles(writer, roles) to add a CONSOLE ACCESS column and use output.Yes/output.No, and adds Ginkgo tests for formatting and ListOCMRoles enrichment and sorting. 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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: 1
🧹 Nitpick comments (1)
cmd/list/ocmroles/cmd_test.go (1)
62-121: ⚡ Quick winTighten column-specific assertions for
printOCMRoles.Current checks for
"Yes"/"No"are ambiguous and can pass from other columns, which weakens verification ofCONSOLE ACCESS.Suggested assertion pattern
+import "strings" ... - dataLine := string(lines[1]) - Expect(dataLine).To(ContainSubstring("No")) + cols := strings.Split(string(lines[1]), "\t") + Expect(cols).To(HaveLen(6)) + Expect(cols[5]).To(Equal("No"))As per coding guidelines
**/*_test.go: Flag weak tests that only restate implementation or changes that weaken existing assertions.🤖 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/list/ocmroles/cmd_test.go` around lines 62 - 121, Tests for printOCMRoles assert presence of "Yes"/"No" globally which can match other columns; update the three specs ("Shows No in CONSOLE ACCESS when NoConsole is true", "Shows Yes in CONSOLE ACCESS when NoConsole is false", and "Shows AWS Managed correctly") to assert the CONSOLE ACCESS column specifically by parsing the printed table row into columns (e.g., split dataLine on the same delimiter printOCMRoles uses or use a regex capturing columns) and then compare the exact column value for console access (and similarly the Managed column), referencing printOCMRoles, the lines slice and dataLine variables to locate where to change assertions.
🤖 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/list/ocmroles/cmd.go`:
- Around line 95-97: The call to writer.Flush() in cmd/list/ocmroles/cmd.go
ignores its returned error; update the code after printOCMRoles(writer,
ocmRoles) to capture the error from writer.Flush(), and if non-nil report it via
the package's reporter (e.g., reporter.Errorf or reporter.Error) with a clear
message including the error, then exit with the same behavior used elsewhere in
this package (os.Exit(1) or the established exit helper). Ensure you reference
the same reporter and exit pattern used by nearby commands so handling is
consistent with printOCMRoles and other cmd/*.go files.
---
Nitpick comments:
In `@cmd/list/ocmroles/cmd_test.go`:
- Around line 62-121: Tests for printOCMRoles assert presence of "Yes"/"No"
globally which can match other columns; update the three specs ("Shows No in
CONSOLE ACCESS when NoConsole is true", "Shows Yes in CONSOLE ACCESS when
NoConsole is false", and "Shows AWS Managed correctly") to assert the CONSOLE
ACCESS column specifically by parsing the printed table row into columns (e.g.,
split dataLine on the same delimiter printOCMRoles uses or use a regex capturing
columns) and then compare the exact column value for console access (and
similarly the Managed column), referencing printOCMRoles, the lines slice and
dataLine variables to locate where to change assertions.
🪄 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: 35396420-383e-4ac8-969e-a29f055b18f0
📒 Files selected for processing (4)
cmd/list/ocmroles/cmd.gocmd/list/ocmroles/cmd_test.gopkg/aws/policies.gopkg/aws/tags/tags.go
| printOCMRoles(writer, ocmRoles) | ||
| writer.Flush() | ||
| } |
There was a problem hiding this comment.
Handle tabwriter flush errors before exiting.
writer.Flush() returns an error; currently failures are ignored, so output issues can be silently missed.
Suggested fix
printOCMRoles(writer, ocmRoles)
- writer.Flush()
+ if err := writer.Flush(); err != nil {
+ r.Reporter.Errorf("Failed to flush ocm roles output: %v", err)
+ os.Exit(1)
+ }As per coding guidelines cmd/**/*.go: Keep error handling consistent with reporter usage, exit behavior, and surrounding package messages.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| printOCMRoles(writer, ocmRoles) | |
| writer.Flush() | |
| } | |
| printOCMRoles(writer, ocmRoles) | |
| if err := writer.Flush(); err != nil { | |
| r.Reporter.Errorf("Failed to flush ocm roles output: %v", err) | |
| 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 `@cmd/list/ocmroles/cmd.go` around lines 95 - 97, The call to writer.Flush() in
cmd/list/ocmroles/cmd.go ignores its returned error; update the code after
printOCMRoles(writer, ocmRoles) to capture the error from writer.Flush(), and if
non-nil report it via the package's reporter (e.g., reporter.Errorf or
reporter.Error) with a clear message including the error, then exit with the
same behavior used elsewhere in this package (os.Exit(1) or the established exit
helper). Ensure you reference the same reporter and exit pattern used by nearby
commands so handling is consistent with printOCMRoles and other cmd/*.go files.
There was a problem hiding this comment.
I would leave writer.Flush() out of this PR because cmd/list/accountroles/cmd.go and the other tabwriter list commands follow the same pattern today. If we want to harden that path, it is better as a small follow-up done consistently across the command area.
There was a problem hiding this comment.
@olucasfreitas, that's a fair point — patching it in isolation here would create inconsistency rather than improve it. Deferring to a dedicated follow-up that covers all tabwriter list commands makes sense.
Would you like me to open a GitHub issue to track the hardening of writer.Flush() error handling consistently across the cmd/list/ tabwriter commands?
| // AdminRole tags the role as admin (true/false) | ||
| const AdminRole = prefix + "admin_role" | ||
|
|
||
| const NoConsoleRole = prefix + "no_console_role" |
There was a problem hiding this comment.
nitpicking: would make sense changing it to a positive boolean? like console = true, has console access.
There was a problem hiding this comment.
@amandahla I have kept the internal structures aligned with the --no-console flag, so for example if customers run rosa list ocm-roles -o json the field there says NoConsole:Yes which I think helps keeps everything aligned and it's the pattern we have for Admin.
WDYT?
There was a problem hiding this comment.
--no-console works fine for the input flag, but pushing NoConsole into aws.Role changes the shared output model. On this branch json.Marshal([]aws.Role{{RoleName:"example"}}) prints [{"RoleName":"example","NoConsole":""}], so every []aws.Role structured output path picks up the new field.
robpblake
force-pushed
the
ocm-24429-list-ocm-role
branch
from
01f4009 to
46bb3dd
Compare
robpblake
force-pushed
the
ocm-24429-list-ocm-role
branch
from
46bb3dd to
118c868
Compare
openshift-ci
Bot
added
the
needs-rebase
robpblake
force-pushed
the
ocm-24429-list-ocm-role
branch
from
118c868 to
95c2aa1
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
Actionable comments posted: 0 |
|
@robpblake when you can, please fill out the PR description following our template, the team uses it to gather as much context as we can for the PR and for future testing |
olucasfreitas
left a comment
olucasfreitas
left a comment
There was a problem hiding this comment.
Structured output here will now emit the new NoConsole field because aws.Role is encoded directly. That leaks an internal inverted flag name instead of the console-access state the PR is trying to show.
robpblake
force-pushed
the
ocm-24429-list-ocm-role
branch
from
95c2aa1 to
3950091
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
robpblake
force-pushed
the
ocm-24429-list-ocm-role
branch
from
3950091 to
484505b
Compare
|
@olucasfreitas / @amandahla This is ready for your review when convenient. cheers, Rob |
|
@robpblake: 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. |
|
/approve @amandahla unhold when you have taken a last look here |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas, robpblake 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 |
3 participants
Adds to the output of
rosa list ocm-rolea field that allows customers to see if theOCM Rolethey have configured has theno-consoleprofile.Example output
PR Summary
This PR updates the
rosa list ocm-rolecommand to show if theOCM Rolethe customer has configured has can be used to deploy clusters viaconsole.redhat.com. This field is captured via theCONSOLE ACCESScolumn in the output, which displaysNoif the user has a--no-consoleversion of theOCM Roleconfigured, andyesotherwiseExamples of the various forms of role showing in the output:
Related Issues and PRs
Type of Change
Previous Behavior
Prior to this behavior, there was no column indicating if the
OCM Rolehad console accessBehavior After This Change
The column is now present if the user has configured a no-console
OCM RoleHow to Test (Step-by-Step)
The easiest way to test is to manually apply Tags to your IAM role for the
OCM Role.Test Steps
OCM Rolewithrosa_no_console_role:true. Runrosa list ocm-roleand see that theCONSOLE ACCESScolumn displaysNorosa_no_console_roletag from theOCM Role. Runrosa list ocm-roleagain and see that theCONSOLE_ACCESScolumn displaysYesExpected Results
Described above
Proof of the Fix
PR Summary
This PR updates the
rosa list ocm-rolecommand to show if theOCM Rolethe customer has configured has can be used to deploy clusters viaconsole.redhat.com. This field is captured via theCONSOLE ACCESScolumn in the output, which displaysNoif the user has a--no-consoleversion of theOCM Roleconfigured, andyesotherwiseExamples of the various forms of role showing in the output:
Additionally, JSON output for various commands where the internal struct is deserialized:
Breaking Changes
Breaking Change Details / Migration Plan
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit