feat: add --no-wait flag for CloudFormation stack operations#60
Open
cdoan1 wants to merge 2 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1 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
WalkthroughCloudFormation operations ( ChangesAsync CloudFormation Operations
Sequence DiagramsequenceDiagram
participant User
participant Cmd as Command Handler
participant Svc as Service Layer
participant CFN as CloudFormation Client
participant AWS as AWS CloudFormation
rect rgba(100, 200, 100, 0.5)
Note over User,AWS: Synchronous Operation (default --no-wait=false)
User->>Cmd: create --stack mystack
Cmd->>Svc: CreateRequest{NoWait: false}
Svc->>CFN: CreateStack{NoWait: false}
CFN->>AWS: CreateStack API
AWS-->>CFN: StackId
CFN->>CFN: Poll DescribeStacks
AWS-->>CFN: Status: CREATE_COMPLETE
CFN->>AWS: DescribeStackResources
AWS-->>CFN: Outputs
CFN-->>Svc: StackOutput{StackId, Outputs}
Svc-->>Cmd: {StackId, Outputs}
Cmd->>User: Created successfully! Outputs: [...]
end
rect rgba(200, 100, 100, 0.5)
Note over User,AWS: Asynchronous Operation (--no-wait=true)
User->>Cmd: create --stack mystack --no-wait
Cmd->>Svc: CreateRequest{NoWait: true}
Svc->>CFN: CreateStack{NoWait: true}
CFN->>AWS: CreateStack API
AWS-->>CFN: StackId
CFN-->>Svc: StackOutput{StackId, Outputs: nil}
Svc-->>Cmd: {StackId, Outputs: nil}
Cmd->>User: Stack creation submitted! StackId: xyz (Check status: describe ...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/commands/clusteriam/create.go (1)
136-174:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSkip OIDC setup when
--no-waitflag is used alongside--oidc-issuer-url.When both
--no-waitand--oidc-issuer-urlare provided, the IAM stack is created asynchronously (lines 136+), but the code immediately attempts to update that stack's trust policies viaupdateIAMTrustPolicies()in theCreateOIDCservice function. AWS CloudFormation does not permitUpdateStackoperations on stacks inCREATE_IN_PROGRESSstate—the stack must first reachCREATE_COMPLETE. This causes the update to fail while the IAM stack is still being created.💡 Suggested guard for the async path
// If OIDC issuer URL was provided, also create the OIDC provider stack if opts.oidcIssuerURL != "" { + if opts.noWait { + fmt.Println("IAM stack creation submitted.") + fmt.Println("Skipping automatic OIDC provider creation in --no-wait mode because IAM stack may still be in progress.") + fmt.Println("Run 'rosactl cluster-oidc create' after IAM stack creation completes.") + return nil + } + fmt.Println() fmt.Println("OIDC issuer URL provided — also creating OIDC provider...") fmt.Println()🤖 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/clusteriam/create.go` around lines 136 - 174, The CreateOIDC flow is attempting to perform an UpdateStack (via updateIAMTrustPolicies) while the IAM CloudFormation stack may still be in CREATE_IN_PROGRESS when --no-wait is used; change clusteroidc.CreateOIDC (and any helper that calls updateIAMTrustPolicies) to skip the trust-policy update when CreateOIDCRequest.NoWait is true (or detect stack state CREATE_IN_PROGRESS and return success without calling updateIAMTrustPolicies), and instead return the submitted StackID/Outputs immediately so the caller (internal/commands/clusteriam/create.go) can report the async submission; ensure the NoWait code path never calls updateIAMTrustPolicies to avoid UpdateStack-on-CREATE_IN_PROGRESS failures.
🤖 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.
Outside diff comments:
In `@internal/commands/clusteriam/create.go`:
- Around line 136-174: The CreateOIDC flow is attempting to perform an
UpdateStack (via updateIAMTrustPolicies) while the IAM CloudFormation stack may
still be in CREATE_IN_PROGRESS when --no-wait is used; change
clusteroidc.CreateOIDC (and any helper that calls updateIAMTrustPolicies) to
skip the trust-policy update when CreateOIDCRequest.NoWait is true (or detect
stack state CREATE_IN_PROGRESS and return success without calling
updateIAMTrustPolicies), and instead return the submitted StackID/Outputs
immediately so the caller (internal/commands/clusteriam/create.go) can report
the async submission; ensure the NoWait code path never calls
updateIAMTrustPolicies to avoid UpdateStack-on-CREATE_IN_PROGRESS failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 1d5e3925-daac-4df2-a767-2e8a2d767a51
📒 Files selected for processing (13)
internal/aws/cloudformation/client.gointernal/aws/cloudformation/stack.gointernal/commands/bootstrap/create.gointernal/commands/bootstrap/delete.gointernal/commands/clusteriam/create.gointernal/commands/clusteriam/delete.gointernal/commands/clusteroidc/create.gointernal/commands/clusteroidc/delete.gointernal/commands/clustervpc/create.gointernal/commands/clustervpc/delete.gointernal/services/clusteriam/service.gointernal/services/clusteroidc/service.gointernal/services/clustervpc/service.go
openshift-ci
Bot
added
the
needs-rebase
Add --no-wait flag to all create and delete subcommands to allow non-blocking CloudFormation operations. When set, the CLI submits the stack operation and returns immediately without waiting for completion. Changes: - CloudFormation client: Add NoWait field to CreateStackParams, UpdateStackParams, and DeleteStack signature - Service layer: Thread NoWait through all Create/Delete request structs (clustervpc, clusteriam, clusteroidc) - Command layer: Add --no-wait flag to all create/delete commands and adjust output messages accordingly This reduces CI pipeline time by allowing async stack teardown in pre-merge e2e tests while preserving blocking behavior by default for reliability. Fixes: ROSAENG-1071 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
needs-rebase
Make --no-wait=true the default for all CloudFormation operations to return immediately without blocking. Users can override with --no-wait=false to wait for completion. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/commands/clusteriam/create.go (1)
119-122: ⚡ Quick winAdd user guidance for checking stack status when using
--no-wait.When the command returns immediately, users receive no guidance on how to verify completion. CloudFormation operations can take 5–15 minutes, and users may need to confirm success before proceeding with dependent operations.
Consider adding actionable next steps, such as:
- AWS CLI command to check stack status:
aws cloudformation describe-stacks --stack-name rosa-{cluster-name}-iam --region {region}- Link to AWS Console CloudFormation page
- Expected completion time estimate
💡 Suggested enhancement
if opts.noWait { fmt.Println("Stack creation submitted!") fmt.Printf(" Stack ID: %s\n", resp.StackID) + fmt.Println() + fmt.Println("To check stack status, run:") + fmt.Printf(" aws cloudformation describe-stacks --stack-name rosa-%s-iam --region %s\n", opts.clusterName, opts.region) } else {🤖 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/clusteriam/create.go` around lines 119 - 122, When the `opts.noWait` flag is true in the create.go file, users receive the stack creation confirmation with a Stack ID but lack guidance on verifying completion. After printing the Stack ID in the noWait block, add additional output that includes an example AWS CLI command to check the stack status (using aws cloudformation describe-stacks with the stack name and region), mention the expected CloudFormation operation time frame (5-15 minutes), and optionally reference the AWS Console CloudFormation page where users can monitor progress. This provides actionable next steps for users who cannot wait for the operation to complete.
🤖 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/clusteriam/create.go`:
- Line 60: The BoolVar flag definition for noWait in the create command has a
default value of true, which contradicts the PR objective to maintain blocking
behavior by default. Change the default value from true to false in the BoolVar
call so that the command blocks and waits for stack creation to complete by
default, allowing users to opt-in to non-blocking behavior by explicitly passing
the --no-wait flag.
---
Nitpick comments:
In `@internal/commands/clusteriam/create.go`:
- Around line 119-122: When the `opts.noWait` flag is true in the create.go
file, users receive the stack creation confirmation with a Stack ID but lack
guidance on verifying completion. After printing the Stack ID in the noWait
block, add additional output that includes an example AWS CLI command to check
the stack status (using aws cloudformation describe-stacks with the stack name
and region), mention the expected CloudFormation operation time frame (5-15
minutes), and optionally reference the AWS Console CloudFormation page where
users can monitor progress. This provides actionable next steps for users who
cannot wait for the operation to complete.
🪄 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: ccb2c380-4b1d-415f-b67d-f5d116d7bfab
📒 Files selected for processing (8)
internal/commands/bootstrap/create.gointernal/commands/bootstrap/delete.gointernal/commands/clusteriam/create.gointernal/commands/clusteriam/delete.gointernal/commands/clusteroidc/create.gointernal/commands/clusteroidc/delete.gointernal/commands/clustervpc/create.gointernal/commands/clustervpc/delete.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/commands/clusteroidc/delete.go
- internal/commands/bootstrap/create.go
- internal/commands/clustervpc/delete.go
- internal/commands/clusteroidc/create.go
- internal/commands/bootstrap/delete.go
- internal/commands/clustervpc/create.go
- internal/commands/clusteriam/delete.go
|
|
||
| cmd.Flags().StringVar(&opts.oidcIssuerURL, "oidc-issuer-url", "", "OIDC issuer URL (optional — also creates OIDC provider if provided)") | ||
| cmd.Flags().StringVar(&opts.region, "region", "", "AWS region (required)") | ||
| cmd.Flags().BoolVar(&opts.noWait, "no-wait", true, "Return immediately without waiting for stack creation to complete") |
There was a problem hiding this comment.
Critical: default contradicts PR objectives and breaks reliability guarantee.
The default value true makes the command return immediately without waiting for stack creation, contradicting the PR description: "maintaining blocking behavior by default to preserve reliability." This inverts the intended behavior and will break existing workflows that depend on blocking completion.
The primary motivation is to reduce CI pipeline time, but that should be opt-in (--no-wait) rather than the default. The commit message acknowledges the default is true, which conflicts with the stated design intent in the PR objectives.
🔧 Proposed fix
- cmd.Flags().BoolVar(&opts.noWait, "no-wait", true, "Return immediately without waiting for stack creation to complete")
+ cmd.Flags().BoolVar(&opts.noWait, "no-wait", false, "Return immediately without waiting for stack creation to complete")📝 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
| cmd.Flags().BoolVar(&opts.noWait, "no-wait", true, "Return immediately without waiting for stack creation to complete") | |
| cmd.Flags().BoolVar(&opts.noWait, "no-wait", false, "Return immediately without waiting for stack creation to complete") |
🤖 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/clusteriam/create.go` at line 60, The BoolVar flag
definition for noWait in the create command has a default value of true, which
contradicts the PR objective to maintain blocking behavior by default. Change
the default value from true to false in the BoolVar call so that the command
blocks and waits for stack creation to complete by default, allowing users to
opt-in to non-blocking behavior by explicitly passing the --no-wait flag.
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.
Add --no-wait flag to all create and delete subcommands to allow non-blocking CloudFormation operations. When set, the CLI submits the stack operation and returns immediately without waiting for completion.
Changes:
This reduces CI pipeline time by allowing async stack teardown in pre-merge e2e tests while preserving blocking behavior by default for reliability.
Fixes: ROSAENG-1071
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit
New Features
--no-waitflag to bootstrap, cluster-iam, cluster-oidc, and cluster-vpc create and delete commands (defaulting to enabled). When set, commands now submit the CloudFormation operation immediately and return stack IDs for progress tracking instead of waiting to complete.Bug Fixes