feat: add nodepool commands and align cluster spec with HyperShift CRD#92
Open
typeid wants to merge 1 commit into
Open
feat: add nodepool commands and align cluster spec with HyperShift CRD#92typeid wants to merge 1 commit into
typeid wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: typeid 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughAdds AWS SigV4-backed ChangesCluster and node pool workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CreateCommand
participant AWS
participant ClusterAPI
participant PlatformAPI
User->>CreateCommand: run nodepool create
CreateCommand->>AWS: load credentials and discover infrastructure
CreateCommand->>ClusterAPI: fetch cluster spec when subnet discovery is needed
ClusterAPI-->>CreateCommand: return cluster specification
CreateCommand->>PlatformAPI: send signed node pool creation request
PlatformAPI-->>CreateCommand: return created node pool
CreateCommand-->>User: print JSON or creation summary
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (2)
internal/commands/nodepool/create.go (2)
170-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResponse body parsed twice depending on output branch.
json.Unmarshal(body, &result)is called once for thejsonoutput path and again (into an identically-typedmap[string]interface{}) for the default text path. Parse once and branch on rendering only.♻️ Parse once, render per format
+ var result map[string]interface{} + if err := json.Unmarshal(body, &result); err != nil { + return fmt.Errorf("failed to parse response: %w", err) + } + if opts.output == "json" { - var result map[string]interface{} - if err := json.Unmarshal(body, &result); err != nil { - return fmt.Errorf("failed to parse JSON response: %w", err) - } prettyJSON, _ := json.MarshalIndent(result, "", " ") fmt.Println(string(prettyJSON)) return nil } - var result map[string]interface{} - if err := json.Unmarshal(body, &result); err != nil { - return fmt.Errorf("failed to parse response: %w", err) - } - fmt.Fprintf(os.Stderr, "\n✓ NodePool created successfully\n")🤖 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 `@internal/commands/nodepool/create.go` around lines 170 - 183, Parse the response body only once before the output-format branch, storing it in the shared result map and returning the existing parse error on failure. Then, in the json branch, marshal and print that parsed result; keep the default path rendering the same result without calling json.Unmarshal again.
76-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated AWS config/credentials/region resolution across all three commands.
This exact block (load config → retrieve credentials → default region to
us-east-1) is repeated verbatim inlist.go(lines 84-97) anddelete.go(lines 37-50). Since all four files sharepackage nodepool, this is a straightforward extraction opportunity.♻️ Suggested shared helper (e.g., in api.go)
func resolveCredentials(ctx context.Context) (awssdk.Credentials, string, error) { cfg, err := aws.NewConfig(ctx) if err != nil { return awssdk.Credentials{}, "", fmt.Errorf("failed to load AWS config: %w", err) } creds, err := cfg.Credentials.Retrieve(ctx) if err != nil { return awssdk.Credentials{}, "", fmt.Errorf("failed to retrieve AWS credentials: %w", err) } region := cfg.Region if region == "" { region = "us-east-1" } return creds, region, nil }Then in each
run*function:- cfg, err := aws.NewConfig(ctx) - if err != nil { - return fmt.Errorf("failed to load AWS config: %w", err) - } - - creds, err := cfg.Credentials.Retrieve(ctx) - if err != nil { - return fmt.Errorf("failed to retrieve AWS credentials: %w", err) - } - - region := cfg.Region - if region == "" { - region = "us-east-1" - } + creds, region, err := resolveCredentials(ctx) + if err != nil { + return err + }🤖 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 `@internal/commands/nodepool/create.go` around lines 76 - 89, Extract the repeated AWS config, credential retrieval, and default-region logic from the run functions in create, list, and delete into a shared package-level helper such as resolveCredentials. Have it return credentials, the resolved region, and wrapped errors, then update each run function to call the helper and remove the duplicated block.
🤖 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 `@internal/commands/nodepool/create.go`:
- Around line 100-115: Propagate CloudFormation lookup failures instead of
silently ignoring them in the instance profile and security group discovery
block. Update the DescribeStack calls within the node pool creation logic to
return or wrap the underlying error immediately with context identifying the
relevant stack and lookup, while preserving successful output assignment.
- Around line 60-64: Add bounds validation for the replicas option in the create
command’s RunE handler before invoking the creation logic, rejecting zero and
negative values with a clear error. Follow the existing validation pattern used
by list.go while referencing the replicas field and create command flow.
In `@internal/services/cluster/service.go`:
- Around line 93-123: Remove the unused WorkerInstanceProfileName lookup and
validation from the spec-generation function, since workerInstanceProfile is not
referenced in the generated payload. Keep the existing IAM role mappings and
spec construction unchanged unless the profile is explicitly required by the
API, in which case add it to the appropriate spec field and retain the
validation.
---
Nitpick comments:
In `@internal/commands/nodepool/create.go`:
- Around line 170-183: Parse the response body only once before the
output-format branch, storing it in the shared result map and returning the
existing parse error on failure. Then, in the json branch, marshal and print
that parsed result; keep the default path rendering the same result without
calling json.Unmarshal again.
- Around line 76-89: Extract the repeated AWS config, credential retrieval, and
default-region logic from the run functions in create, list, and delete into a
shared package-level helper such as resolveCredentials. Have it return
credentials, the resolved region, and wrapped errors, then update each run
function to call the helper and remove the duplicated block.
🪄 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-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aafdca30-aa0e-48fb-9d8a-0985866ec4c6
📒 Files selected for processing (9)
internal/commands/cluster/create.gointernal/commands/cluster/list.gointernal/commands/nodepool/api.gointernal/commands/nodepool/create.gointernal/commands/nodepool/delete.gointernal/commands/nodepool/list.gointernal/commands/nodepool/nodepool.gointernal/commands/root.gointernal/services/cluster/service.go
typeid
added
do-not-merge/work-in-progress
Add rosactl nodepool create/list/delete subcommands that talk to the platform API with SigV4-signed requests. The create command auto-discovers subnet, instance profile, and security groups from the cluster spec and CloudFormation stacks when flags are omitted. Align the cluster configuration with the HyperShift ClusterSpec CRD: - Nest IAM role ARNs under platform.aws.rolesRef - Send privateSubnetIds as an array - Rename cloudUrl to oidcIssuerURL throughout Harden commands against malformed or malicious input: - Path-escape IDs interpolated into API URLs - Return errors on JSON unmarshal failures instead of swallowing them - Validate pagination flags (limit 1-100, offset >= 0) - Fail fast on missing CloudFormation stack outputs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
rosactl nodepool create/list/deletesubcommands with SigV4-signed platform API requestsTest plan
rosactl nodepool createwith auto-discovery from cluster specrosactl nodepool createwith explicit--subnet-id,--instance-profile,--security-groupsrosactl nodepool list --cluster-id <id>table and JSON outputrosactl nodepool listwith invalid--limit/--offsetvalues rejectedrosactl nodepool delete <id>rosactl cluster createshows "OIDC Issuer URL" (not "Cloud URL") in summaryrosactl cluster create --dry-rungenerates valid ClusterSpec JSON🤖 Generated with Claude Code
Summary by CodeRabbit
nodepoolcommands to create, list, and delete ROSA hosted cluster node pools.--limit/--offset, showing key node pool fields.