ROSAENG-63260 | refactor: Create reusable request/response conventions - #3452
Draft
markirish wants to merge 1 commit into
Draft
ROSAENG-63260 | refactor: Create reusable request/response conventions#3452markirish wants to merge 1 commit into
markirish wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: markirish The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe change documents workflow 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/iamserviceaccount/client.go (1)
40-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused producer-side interface abstraction.
IAMServiceAccountServicehas only one implementation and no production caller uses it. Move this to a concreteServiceinpkg/iamserviceaccount; if an external consumer needs an interface, define a minimal interface at that consumer boundary instead.🤖 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/iamserviceaccount/client.go` around lines 40 - 46, Remove the unused IAMServiceAccountService interface and define the workflow on a concrete Service type in pkg/iamserviceaccount, updating CreateIAMServiceAccount references and construction accordingly. Preserve the existing CreateIAMServiceAccount signature and behavior; any interface needed by external consumers should be declared at their boundary.Source: Coding guidelines
🤖 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/iamserviceaccount/create.go`:
- Around line 114-117: Update the request validation flow in the create
operation to check whether req is nil before calling req.Validate(), and return
an appropriate validation error instead of dereferencing it. Add a focused spec
covering a nil request passed through this call path, while preserving the
existing validation behavior for non-nil requests.
- Around line 141-150: Update the concrete IAM client methods EnsureRole,
AttachRolePolicy, and PutRolePolicy to accept ctx context.Context, then
propagate that context to every AWS SDK IAM request and paginator page call
instead of using context.Background() or context.TODO(). Update all callers,
including the service-account creation flow, to pass the existing caller context
through.
- Around line 51-83: Update CreateIAMServiceAccountRequest.Validate to reject
blank entries in PolicyARNs and validate that a non-empty InlinePolicy contains
valid JSON before returning success. Ensure these checks occur before role
creation through the existing validation path, and add specs verifying invalid
policy inputs fail without making IAM client calls.
---
Nitpick comments:
In `@pkg/iamserviceaccount/client.go`:
- Around line 40-46: Remove the unused IAMServiceAccountService interface and
define the workflow on a concrete Service type in pkg/iamserviceaccount,
updating CreateIAMServiceAccount references and construction accordingly.
Preserve the existing CreateIAMServiceAccount signature and behavior; any
interface needed by external consumers should be declared at their boundary.
🪄 Autofix
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: Pro Plus
Run ID: 09987a68-a0a5-4b33-87b5-779254be2e3f
📒 Files selected for processing (6)
AGENTS.mdguidelines/ARCHITECTURE.mdguidelines/workflow-conventions.mdpkg/iamserviceaccount/client.gopkg/iamserviceaccount/create.gopkg/iamserviceaccount/create_test.go
Comment on lines
+51
to
+83
| func (r *CreateIAMServiceAccountRequest) Validate() error { | ||
| if r.ClusterName == "" { | ||
| return fmt.Errorf("cluster name is required") | ||
| } | ||
| if r.OIDCProviderARN == "" { | ||
| return fmt.Errorf("OIDC provider ARN is required") | ||
| } | ||
| if len(r.ServiceAccounts) == 0 { | ||
| return fmt.Errorf("at least one service account is required") | ||
| } | ||
| for _, sa := range r.ServiceAccounts { | ||
| if err := ValidateServiceAccountName(sa.Name); err != nil { | ||
| return fmt.Errorf("invalid service account name %q: %w", sa.Name, err) | ||
| } | ||
| if err := ValidateNamespaceName(sa.Namespace); err != nil { | ||
| return fmt.Errorf("invalid namespace %q for service account %q: %w", sa.Namespace, sa.Name, err) | ||
| } | ||
| } | ||
| if len(r.PolicyARNs) == 0 && r.InlinePolicy == "" { | ||
| return fmt.Errorf("at least one policy ARN or inline policy is required") | ||
| } | ||
| if r.RoleName == "" && len(r.ServiceAccounts) > 1 { | ||
| return fmt.Errorf("role name is required when specifying multiple service accounts") | ||
| } | ||
| if r.IsGovcloud { | ||
| if r.AccountID == "" { | ||
| return fmt.Errorf("account ID is required for GovCloud environments") | ||
| } | ||
| if r.Partition == "" { | ||
| return fmt.Errorf("partition is required for GovCloud environments") | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate each policy input before role creation.
PolicyARNs: []string{""} passes validation. A malformed non-empty InlinePolicy also passes validation. The workflow then calls EnsureRole before IAM rejects the policy input. This can leave a created or modified role after the command returns an error.
Reject blank policy ARN entries. Reject invalid inline-policy JSON before calling EnsureRole. Add specs that confirm these inputs make no IAM client calls.
Proposed validation change
import (
"context"
+ "encoding/json"
"fmt"
+ "strings"
)
- if len(r.PolicyARNs) == 0 && r.InlinePolicy == "" {
+ if len(r.PolicyARNs) == 0 && strings.TrimSpace(r.InlinePolicy) == "" {
return fmt.Errorf("at least one policy ARN or inline policy is required")
}
+ for _, policyARN := range r.PolicyARNs {
+ if strings.TrimSpace(policyARN) == "" {
+ return fmt.Errorf("policy ARN cannot be empty")
+ }
+ }
+ if r.InlinePolicy != "" && !json.Valid([]byte(r.InlinePolicy)) {
+ return fmt.Errorf("inline policy must be valid JSON")
+ }📝 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.
Suggested change
| func (r *CreateIAMServiceAccountRequest) Validate() error { | |
| if r.ClusterName == "" { | |
| return fmt.Errorf("cluster name is required") | |
| } | |
| if r.OIDCProviderARN == "" { | |
| return fmt.Errorf("OIDC provider ARN is required") | |
| } | |
| if len(r.ServiceAccounts) == 0 { | |
| return fmt.Errorf("at least one service account is required") | |
| } | |
| for _, sa := range r.ServiceAccounts { | |
| if err := ValidateServiceAccountName(sa.Name); err != nil { | |
| return fmt.Errorf("invalid service account name %q: %w", sa.Name, err) | |
| } | |
| if err := ValidateNamespaceName(sa.Namespace); err != nil { | |
| return fmt.Errorf("invalid namespace %q for service account %q: %w", sa.Namespace, sa.Name, err) | |
| } | |
| } | |
| if len(r.PolicyARNs) == 0 && r.InlinePolicy == "" { | |
| return fmt.Errorf("at least one policy ARN or inline policy is required") | |
| } | |
| if r.RoleName == "" && len(r.ServiceAccounts) > 1 { | |
| return fmt.Errorf("role name is required when specifying multiple service accounts") | |
| } | |
| if r.IsGovcloud { | |
| if r.AccountID == "" { | |
| return fmt.Errorf("account ID is required for GovCloud environments") | |
| } | |
| if r.Partition == "" { | |
| return fmt.Errorf("partition is required for GovCloud environments") | |
| } | |
| } | |
| return nil | |
| func (r *CreateIAMServiceAccountRequest) Validate() error { | |
| if r.ClusterName == "" { | |
| return fmt.Errorf("cluster name is required") | |
| } | |
| if r.OIDCProviderARN == "" { | |
| return fmt.Errorf("OIDC provider ARN is required") | |
| } | |
| if len(r.ServiceAccounts) == 0 { | |
| return fmt.Errorf("at least one service account is required") | |
| } | |
| for _, sa := range r.ServiceAccounts { | |
| if err := ValidateServiceAccountName(sa.Name); err != nil { | |
| return fmt.Errorf("invalid service account name %q: %w", sa.Name, err) | |
| } | |
| if err := ValidateNamespaceName(sa.Namespace); err != nil { | |
| return fmt.Errorf("invalid namespace %q for service account %q: %w", sa.Namespace, sa.Name, err) | |
| } | |
| } | |
| if len(r.PolicyARNs) == 0 && strings.TrimSpace(r.InlinePolicy) == "" { | |
| return fmt.Errorf("at least one policy ARN or inline policy is required") | |
| } | |
| for _, policyARN := range r.PolicyARNs { | |
| if strings.TrimSpace(policyARN) == "" { | |
| return fmt.Errorf("policy ARN cannot be empty") | |
| } | |
| } | |
| if r.InlinePolicy != "" && !json.Valid([]byte(r.InlinePolicy)) { | |
| return fmt.Errorf("inline policy must be valid JSON") | |
| } | |
| if r.RoleName == "" && len(r.ServiceAccounts) > 1 { | |
| return fmt.Errorf("role name is required when specifying multiple service accounts") | |
| } | |
| if r.IsGovcloud { | |
| if r.AccountID == "" { | |
| return fmt.Errorf("account ID is required for GovCloud environments") | |
| } | |
| if r.Partition == "" { | |
| return fmt.Errorf("partition is required for GovCloud environments") | |
| } | |
| } | |
| return nil | |
| } |
🤖 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/iamserviceaccount/create.go` around lines 51 - 83, Update
CreateIAMServiceAccountRequest.Validate to reject blank entries in PolicyARNs
and validate that a non-empty InlinePolicy contains valid JSON before returning
success. Ensure these checks occur before role creation through the existing
validation path, and add specs verifying invalid policy inputs fail without
making IAM client calls.
- Create a document outlining how requests from CLI to core layers should be structured - Outline how responses from core layer to CLI should be structured - Implement a minimal request/response workflow when creating an IAMServiceAccount
markirish
force-pushed
the
ROSAENG-63260-define-reusable-workflow-conventions
branch
from
August 6, 2026 16:15
b13666e to
79b1c0a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
1 participant
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.
PR Summary
Detailed Description of the Issue
Related Issues and PRs
#Type of Change
Previous Behavior
Behavior After This Change
How to Test (Step-by-Step)
Preconditions
Test Steps
Expected Results
Proof of the Fix
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
New Features
Documentation
Tests