Prevent creating stack with name of existing stack#1969
Open
jordanstephens wants to merge 3 commits intomainfrom
Open
Prevent creating stack with name of existing stack#1969jordanstephens wants to merge 3 commits intomainfrom
jordanstephens wants to merge 3 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds duplicate stack name validation to prevent creating stacks with identical names in a project. It introduces a public Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Command
participant Validator as Stack Validator
participant Client as Fabric Client
participant Project as Project State
User->>CLI: Create stack (provide name)
CLI->>Validator: ValidateStackName(name)
Validator-->>CLI: Validation result
alt Name is invalid
CLI-->>User: Error: Invalid stack name
else Name is valid
CLI->>Client: ListStacks(project)
Client->>Project: Query existing stacks
Project-->>Client: Return stacks list
Client-->>CLI: Stacks found
CLI->>Validator: stackExists(name, stacks)
Validator-->>CLI: Existence check result
alt Stack exists
CLI-->>User: Error: Stack already exists
else Stack does not exist
CLI->>Project: Create new stack
Project-->>CLI: Success
CLI-->>User: Stack created
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Comment |
jordanstephens
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cmd/cli/command/stack_test.go (1)
165-184:⚠️ Potential issue | 🟡 MinorTwo table cases aren’t exercising the paths they’re named after.
"missing stack name"callsRunEwith one empty arg, which the CLI can’t actually produce, and"stack already exists"usesexisting-stack, which fails name validation beforestackExists()runs. That leaves both the zero-arg path and the duplicate-name path untested.💡 Suggested change
{ name: "missing stack name", parameters: stacks.Parameters{ Name: "", @@ { name: "stack already exists", parameters: stacks.Parameters{ - Name: "existing-stack", + Name: "existingstack", Provider: client.ProviderAWS, Region: "us-test-2", Mode: modes.ModeAffordable, }, - existingStacks: []*defangv1.Stack{{Name: "existing-stack", Project: ""}}, + existingStacks: []*defangv1.Stack{{Name: "existingstack", Project: ""}}, expectErr: true, }, @@ - err := stackCreateCmd.RunE(stackCreateCmd, []string{tt.parameters.Name}) + var args []string + if tt.parameters.Name != "" { + args = []string{tt.parameters.Name} + } + err := stackCreateCmd.RunE(stackCreateCmd, args)Also applies to: 191-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/stack_test.go` around lines 165 - 184, The two test cases in stack_test.go are not exercising the intended paths: change the "missing stack name" case to call the command's RunE with zero args (simulate CLI invocation with no arguments) so the name-validation path for empty input is hit, and change the "stack already exists" case to use a valid, non-empty stack name that passes name validation (e.g., "existing-stack" kept but ensure the test supplies it as a proper argument) so that stackExists() is reached and triggers the duplicate-name error; update the test invocations around RunE/Execute/command creation in the same file to reflect zero-arg invocation for the first case and valid-arg invocation for the second so the code paths for name missing and duplicate name are both covered.
🧹 Nitpick comments (1)
src/pkg/stacks/stacks_test.go (1)
30-31: UseValidateStackName()here instead of the raw regexp.This test is checking whether
MakeDefaultName()produces a name the package accepts. MatchingStackNamePatterndirectly couples the assertion to the current implementation and will miss any extra rules added inValidateStackName().💡 Suggested change
- if !StackNamePattern.MatchString(result) { - t.Errorf("MakeDefaultName() produced invalid stack name: %q", result) - } + if err := ValidateStackName(result); err != nil { + t.Errorf("MakeDefaultName() produced invalid stack name %q: %v", result, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/stacks/stacks_test.go` around lines 30 - 31, The test currently asserts MakeDefaultName() by matching against StackNamePattern directly; change it to call ValidateStackName(result) and assert that it returns nil (or no error) so the test verifies the full validation logic. Replace the direct StackNamePattern.MatchString(result) check with a call to ValidateStackName(result) and update the failure message to include the returned error (e.g., "MakeDefaultName() produced invalid stack name: %q: %v") so the test fails with the validator's reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/stack_test.go`:
- Around line 25-29: The mock ListStacks implementation
(mockFabricClientWithStacks.ListStacks) ignores ListStacksRequest.Project, so
tests like TestStackExists won't catch code that fails to scope by project;
modify ListStacks to check the incoming request.Project against an expected
project value stored on the mock (e.g., add a field like expectedProject or use
m.project) and return an error (or empty result) when they differ, while still
returning m.listStacksErr when set and returning the stacked response when the
project matches.
In `@src/cmd/cli/command/stack.go`:
- Around line 47-49: The validation call to stacks.ValidateStackName should only
run when the user actually supplied the positional STACK_NAME; currently it runs
unconditionally and rejects the zero-arg interactive flow when stackName == "".
Update the branch around ValidateStackName (the code referencing stackName and
stacks.ValidateStackName) to first check that stackName is non-empty (or that
the positional arg count > 0) and only then call stacks.ValidateStackName,
leaving the empty stackName path to proceed to the interactive survey.
In `@src/pkg/stacks/stacks_test.go`:
- Around line 202-205: The test asserts a platform-specific error string after
calling RemoveInDirectory(".", "nonexistingstack"); instead, check for the
standard not-found sentinel: replace assert.ErrorContains(t, err, "...no such
file...") with a portability-safe assertion like assert.ErrorIs(t, err,
os.ErrNotExist) (or assert.True(t, errors.Is(err, os.ErrNotExist)) if ErrorIs
isn't available) and add the necessary import for os (and errors if used); keep
the existing assert.Error(t, err) or remove it since ErrorIs covers presence.
---
Outside diff comments:
In `@src/cmd/cli/command/stack_test.go`:
- Around line 165-184: The two test cases in stack_test.go are not exercising
the intended paths: change the "missing stack name" case to call the command's
RunE with zero args (simulate CLI invocation with no arguments) so the
name-validation path for empty input is hit, and change the "stack already
exists" case to use a valid, non-empty stack name that passes name validation
(e.g., "existing-stack" kept but ensure the test supplies it as a proper
argument) so that stackExists() is reached and triggers the duplicate-name
error; update the test invocations around RunE/Execute/command creation in the
same file to reflect zero-arg invocation for the first case and valid-arg
invocation for the second so the code paths for name missing and duplicate name
are both covered.
---
Nitpick comments:
In `@src/pkg/stacks/stacks_test.go`:
- Around line 30-31: The test currently asserts MakeDefaultName() by matching
against StackNamePattern directly; change it to call ValidateStackName(result)
and assert that it returns nil (or no error) so the test verifies the full
validation logic. Replace the direct StackNamePattern.MatchString(result) check
with a call to ValidateStackName(result) and update the failure message to
include the returned error (e.g., "MakeDefaultName() produced invalid stack
name: %q: %v") so the test fails with the validator's reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61061d17-cd8e-4771-b5cd-0416519ff04b
📒 Files selected for processing (5)
src/cmd/cli/command/stack.gosrc/cmd/cli/command/stack_test.gosrc/pkg/stacks/stacks.gosrc/pkg/stacks/stacks_test.gosrc/pkg/stacks/wizard.go
Comment on lines
+25
to
+29
| func (m mockFabricClientWithStacks) ListStacks(_ context.Context, _ *defangv1.ListStacksRequest) (*defangv1.ListStacksResponse, error) { | ||
| if m.listStacksErr != nil { | ||
| return nil, m.listStacksErr | ||
| } | ||
| return &defangv1.ListStacksResponse{Stacks: m.existingStacks}, nil |
Contributor
There was a problem hiding this comment.
Make the mock verify the requested project.
This mock ignores ListStacksRequest.Project, so TestStackExists would still pass if stackExists() stopped scoping the RPC to the current project and started rejecting names from other projects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/cli/command/stack_test.go` around lines 25 - 29, The mock ListStacks
implementation (mockFabricClientWithStacks.ListStacks) ignores
ListStacksRequest.Project, so tests like TestStackExists won't catch code that
fails to scope by project; modify ListStacks to check the incoming
request.Project against an expected project value stored on the mock (e.g., add
a field like expectedProject or use m.project) and return an error (or empty
result) when they differ, while still returning m.listStacksErr when set and
returning the stacked response when the project matches.
Comment on lines
+47
to
+49
| if err := stacks.ValidateStackName(stackName); err != nil { | ||
| return fmt.Errorf("invalid stack name %q: %v", stackName, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t reject the zero-arg interactive path here.
When defang stack new is run without STACK_NAME, stackName is still "" at this point, so the command now exits with invalid stack name "" instead of launching the survey. This validation needs to run only when the positional arg was actually provided.
💡 Suggested change
- if err := stacks.ValidateStackName(stackName); err != nil {
- return fmt.Errorf("invalid stack name %q: %v", stackName, err)
- }
+ if stackName != "" {
+ if err := stacks.ValidateStackName(stackName); err != nil {
+ return fmt.Errorf("invalid stack name %q: %v", stackName, err)
+ }
+ }📝 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
| if err := stacks.ValidateStackName(stackName); err != nil { | |
| return fmt.Errorf("invalid stack name %q: %v", stackName, err) | |
| } | |
| if stackName != "" { | |
| if err := stacks.ValidateStackName(stackName); err != nil { | |
| return fmt.Errorf("invalid stack name %q: %v", stackName, err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/cli/command/stack.go` around lines 47 - 49, The validation call to
stacks.ValidateStackName should only run when the user actually supplied the
positional STACK_NAME; currently it runs unconditionally and rejects the
zero-arg interactive flow when stackName == "". Update the branch around
ValidateStackName (the code referencing stackName and stacks.ValidateStackName)
to first check that stackName is non-empty (or that the positional arg count >
0) and only then call stacks.ValidateStackName, leaving the empty stackName path
to proceed to the interactive survey.
Comment on lines
+202
to
+205
| err := RemoveInDirectory(".", "nonexistingstack") | ||
| // expect an error when trying to remove a non-existing stack | ||
| assert.Error(t, err) | ||
| assert.ErrorContains(t, err, "remove .defang/non_existing_stack: no such file or directory") | ||
| assert.ErrorContains(t, err, "remove .defang/nonexistingstack: no such file or directory") |
Contributor
There was a problem hiding this comment.
Avoid asserting the platform-specific os.Remove text.
The exact error string varies by OS and path separator. ErrorIs(..., os.ErrNotExist) keeps the test stable while still verifying the behavior you care about.
💡 Suggested change
err := RemoveInDirectory(".", "nonexistingstack")
// expect an error when trying to remove a non-existing stack
assert.Error(t, err)
- assert.ErrorContains(t, err, "remove .defang/nonexistingstack: no such file or directory")
+ assert.ErrorIs(t, err, os.ErrNotExist)📝 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
| err := RemoveInDirectory(".", "nonexistingstack") | |
| // expect an error when trying to remove a non-existing stack | |
| assert.Error(t, err) | |
| assert.ErrorContains(t, err, "remove .defang/non_existing_stack: no such file or directory") | |
| assert.ErrorContains(t, err, "remove .defang/nonexistingstack: no such file or directory") | |
| err := RemoveInDirectory(".", "nonexistingstack") | |
| // expect an error when trying to remove a non-existing stack | |
| assert.Error(t, err) | |
| assert.ErrorIs(t, err, os.ErrNotExist) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/stacks/stacks_test.go` around lines 202 - 205, The test asserts a
platform-specific error string after calling RemoveInDirectory(".",
"nonexistingstack"); instead, check for the standard not-found sentinel: replace
assert.ErrorContains(t, err, "...no such file...") with a portability-safe
assertion like assert.ErrorIs(t, err, os.ErrNotExist) (or assert.True(t,
errors.Is(err, os.ErrNotExist)) if ErrorIs isn't available) and add the
necessary import for os (and errors if used); keep the existing assert.Error(t,
err) or remove it since ErrorIs covers presence.
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.
Description
Fail to create a stack if the name is invalid. Check if passed by arg or by interactive survey
Linked Issues
Checklist
Summary by CodeRabbit
New Features
Tests