Skip to content

Conversation

@andreadecorte
Copy link
Contributor

>> Going to delete share vpc role
[{0xc0006bfd80 0xc0006bfd90 {}}]
[{0xc000ee0430 0xc000ee0440 {}}]

Fix by looping over list and printing ARN if not nil

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

The DeleteRoleAndPolicy function in pkg/aws/aws_client/role.go was changed to iterate over output.AttachedPolicies and print each PolicyArn only when non-nil, replacing a previous unconditional print of the entire slice.

Changes

Cohort / File(s) Summary
Role policy deletion output formatting
pkg/aws/aws_client/role.go
Replaced an unconditional print of the AttachedPolicies slice with a per-item iteration that nil-checks each PolicyArn before printing in DeleteRoleAndPolicy. The policy-detach loop is unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review DeleteRoleAndPolicy for correct nil checks and formatting.
  • Confirm no behavioral changes in the subsequent detach loop and error handling.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing the printing of attached policies list by iterating and printing ARNs correctly instead of printing pointer addresses.
Description check ✅ Passed The description is directly related to the changeset, showing the current problematic output with pointer addresses and explaining the proposed fix of looping and printing ARNs when not nil.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 395756d and e0c09b5.

📒 Files selected for processing (1)
  • pkg/aws/aws_client/role.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/aws/aws_client/role.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Lint

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/aws/aws_client/role.go (1)

97-101: Consider using the log package for consistency.

The codebase imports github.com/openshift-online/ocm-common/pkg/log (line 9) and uses log.LogError elsewhere (e.g., lines 418, 440). Consider using the log package here instead of fmt.Println for consistent logging practices.

For example:

+	log.LogInfo("Attached policies for role %s:", roleName)
 	for _, attachedPolicy := range output.AttachedPolicies {
 		if attachedPolicy.PolicyArn != nil {
-			fmt.Println(*attachedPolicy.PolicyArn)
+			log.LogInfo("  - %s", *attachedPolicy.PolicyArn)
 		}
 	}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between dbfc861 and 395756d.

📒 Files selected for processing (1)
  • pkg/aws/aws_client/role.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Test (windows-latest)
  • GitHub Check: Test (macos-latest)
  • GitHub Check: Test (ubuntu-latest)
  • GitHub Check: Lint

```
>> Going to delete share vpc role
[{0xc0006bfd80 0xc0006bfd90 {}}]
[{0xc000ee0430 0xc000ee0440 {}}]
```

Fix by looping over list and printing ARN if not nil
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant