fix: check dir is exists before create/clone on world cardinal create command #83
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “graphite/merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request enhance the Changes
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
zulkhair
changed the title
zulkhair
force-pushed
the
daim/check_dir_exist_before_create_clone
branch
from
ac81682
to
657f786
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- common/teacmd/git.go (1 hunks)
🔇 Additional comments (1)
common/teacmd/git.go (1)
50-55: Implementation aligns well with PR objectives and codebase patterns.The changes successfully address the PR objectives by adding the directory existence check before the clone operation. The implementation:
- Provides clear error messages as required
- Uses consistent error handling with the eris package
- Maintains the existing code style
- Has no impact on the rest of the clone process
Let's verify the error handling consistency:
✅ Verification successful
Error handling is consistent throughout the codebase
The verification confirms that the error handling in the new code aligns perfectly with the established pattern across the codebase. All error handling consistently uses the
erispackage with appropriate error wrapping and formatting methods (eris.New,eris.Wrap,eris.Errorf). The new error message follows the same style as other error messages in the codebase, particularly in the same package (common/teacmd/git.go).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent use of error handling package rg "eris\." --type goLength of output: 7274
zulkhair
force-pushed
the
daim/check_dir_exist_before_create_clone
branch
from
657f786
to
a1f6ab6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- cmd/world/root/root_test.go (1 hunks)
- common/teacmd/git.go (1 hunks)
🔇 Additional comments (3)
common/teacmd/git.go (2)
50-54: Implementation meets core requirements.The added directory existence check successfully addresses the PR objective of providing a helpful error message when the target directory already exists.
50-54: 🛠️ Refactor suggestionConsider enhancing error handling robustness.
While the current implementation works, it could be more robust by:
- Distinguishing between files and directories
- Handling permission issues explicitly
- Addressing potential race conditions
Consider implementing this more comprehensive check:
- if _, err := os.Stat(targetDir); err == nil { - return eris.Errorf("Game shard named '%s' already exists in this directory, "+ - "please change the directory or use another name", targetDir) - } + fileInfo, err := os.Stat(targetDir) + if err != nil { + if !os.IsNotExist(err) { + return eris.Wrapf(err, "failed to check if '%s' exists (possible permission issue)", targetDir) + } + } else { + if fileInfo.IsDir() { + return eris.Errorf("game shard named '%s' already exists as a directory, "+ + "please use another name", targetDir) + } + return eris.Errorf("cannot create game shard '%s': a file with this name already exists", + targetDir) + }This enhancement:
- Provides specific error messages for files vs directories
- Explicitly handles permission issues
- Maintains clear user guidance in error messages
Likely invalid or redundant comment.
cmd/world/root/root_test.go (1)
119-120: Verify the necessity of the subdirectory structure.The change to use a subdirectory
sgtappears unrelated to the PR's objective of adding directory existence checks. Could you explain why this directory structure change is necessary?
zulkhair
force-pushed
the
daim/check_dir_exist_before_create_clone
branch
from
a1f6ab6
to
1032212
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
cmd/world/root/root_test.go (1)
Line range hint
120-184: Add test cases for directory existence.While the changes modify the directory structure, they don't directly test the PR's main objective: improved error handling when a directory already exists.
Consider adding a new test function to verify the improved error handling:
func TestCreateWithExistingDirectory(t *testing.T) { // Create a temporary directory parentDir, err := os.MkdirTemp("", "world-cli-test") assert.NilError(t, err) defer os.RemoveAll(parentDir) // Create the target directory that will conflict targetDir := filepath.Join(parentDir, "sgt") err = os.Mkdir(targetDir, 0755) assert.NilError(t, err) // Attempt to create a world in the existing directory teaOut := &bytes.Buffer{} createCmd := getCreateCmd(teaOut) createCmd.SetArgs([]string{targetDir}) // Verify that we get the expected error err = createCmd.Execute() assert.ErrorContains(t, err, "directory already exists") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- cmd/world/root/root_test.go (2 hunks)
- common/teacmd/git.go (1 hunks)
🔇 Additional comments (4)
common/teacmd/git.go (2)
50-54: Implementation meets the basic requirement.The check successfully prevents overwriting existing directories and provides a clear error message to users, which addresses the core issue WORLD-1044.
50-54: 🛠️ Refactor suggestionConsider enhancing the directory check for better error handling.
While the current implementation works, we could make it more robust by distinguishing between different error cases. This would provide even clearer guidance to users.
Consider this enhanced implementation:
- if _, err := os.Stat(targetDir); err == nil { - return eris.Errorf("Game shard named '%s' already exists in this directory, "+ - "please change the directory or use another name", targetDir) - } + fileInfo, err := os.Stat(targetDir) + if err != nil { + if !os.IsNotExist(err) { + return eris.Wrapf(err, "failed to check if '%s' exists (possible permission issue)", targetDir) + } + } else { + if fileInfo.IsDir() { + return eris.Errorf("game shard named '%s' already exists as a directory, "+ + "please use another name", targetDir) + } + return eris.Errorf("cannot create game shard '%s': a file with this name already exists", + targetDir) + }This enhancement:
- Distinguishes between files and directories
- Provides specific error messages for different scenarios
- Explicitly handles permission issues
Likely invalid or redundant comment.
cmd/world/root/root_test.go (2)
120-122: Verify test isolation with subdirectory creation.The test now creates a subdirectory "sgt" under the temporary directory. While this change aligns with the PR's goal of improving directory handling, we should ensure proper cleanup and test isolation.
✅ Verification successful
Test isolation is properly maintained
The usage of "sgt" in both locations follows the same pattern - creating temporary test directories. In
common/docker/client_test.go, it's used as a prefix forMkdirTemp, while in the reviewed code it's a subdirectory under another temporary directory. Both approaches maintain proper test isolation as they:
- Use separate temporary directories
- Follow cleanup patterns
- Don't interfere with each other's execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other tests might be affected by this directory structure change rg -l "sgt" --type goLength of output: 50
Script:
#!/bin/bash # Let's check the content of the found file to understand the usage of "sgt" rg "sgt" -C 3 common/docker/client_test.goLength of output: 185
126-129:⚠️ Potential issueConsider potential test interference.
Changing the working directory without restoring it could affect subsequent tests. Consider adding a deferred cleanup to restore the original working directory.
Apply this diff to ensure proper cleanup:
// Change dir err = os.Chdir(sgtDir) assert.NilError(t, err) + + // Store and restore original directory + origDir, err := os.Getwd() + assert.NilError(t, err) + defer func() { + err = os.Chdir(origDir) + assert.NilError(t, err) + }()Likely invalid or redundant comment.
zulkhair
force-pushed
the
daim/check_dir_exist_before_create_clone
branch
from
1032212
to
e2d9513
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- cmd/world/root/root_test.go (3 hunks)
- common/teacmd/git.go (1 hunks)
🔇 Additional comments (3)
common/teacmd/git.go (2)
50-54: Implementation meets the basic requirements.The directory existence check successfully prevents the unhelpful error message mentioned in WORLD-1044 by providing a clear error message when the target directory already exists.
50-54: 🛠️ Refactor suggestionConsider enhancing the directory check implementation.
While the current implementation works, it could be more robust by:
- Distinguishing between files and directories
- Handling different error cases (e.g., permission issues)
- Providing more specific error messages
The previous review already provided a comprehensive implementation suggestion that addresses these points. Consider implementing those improvements to make the code more robust and maintainable.
cmd/world/root/root_test.go (1)
126-129:⚠️ Potential issueRestore working directory after test completion.
Changing the working directory without restoring it could affect subsequent tests. Consider adding a deferred function to restore the original working directory.
Apply this change:
// Change dir + originalDir, err := os.Getwd() + assert.NilError(t, err) + defer func() { + err = os.Chdir(originalDir) + assert.NilError(t, err) + }() err = os.Chdir(sgtDir) assert.NilError(t, err)Likely invalid or redundant comment.
zulkhair
force-pushed
the
daim/check_dir_exist_before_create_clone
branch
from
e2d9513
to
9e8086f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #83 +/- ##
==========================================
- Coverage 46.87% 46.84% -0.04%
==========================================
Files 56 56
Lines 3541 3546 +5
==========================================
+ Hits 1660 1661 +1
- Misses 1672 1675 +3
- Partials 209 210 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- cmd/world/root/root_test.go (3 hunks)
- common/docker/client_test.go (1 hunks)
- common/teacmd/git.go (1 hunks)
🔇 Additional comments (5)
common/teacmd/git.go (2)
50-54: Basic directory check meets requirements.The implementation successfully addresses the PR objective by checking directory existence and providing a clear, actionable error message to users.
50-54: 🛠️ Refactor suggestionConsider enhancing error handling robustness.
While the current implementation works, the previous review suggestion for more comprehensive error handling remains valid. Consider distinguishing between files and directories, and handling different error cases.
Here's an enhanced implementation:
- if _, err := os.Stat(targetDir); err == nil { - return eris.Errorf("Game shard named '%s' already exists in this directory, "+ - "please change the directory or use another name", targetDir) - } + fileInfo, err := os.Stat(targetDir) + if err != nil { + if !os.IsNotExist(err) { + return eris.Wrapf(err, "failed to check if '%s' exists", targetDir) + } + } else { + if fileInfo.IsDir() { + return eris.Errorf("game shard named '%s' already exists as a directory, "+ + "please use another name", targetDir) + } + return eris.Errorf("cannot create game shard '%s': a file with this name already exists", + targetDir) + }Likely invalid or redundant comment.
common/docker/client_test.go (2)
187-195: LGTM! Changes align with PR objectives.The modifications to use
sgtDirfor both the Git clone operation and configuration setup properly support the PR's goal of improving directory handling. The error handling is maintained, and the test structure remains clear.
187-187: Verify GitCloneCmd implementation.Since this test relies on
teacmd.GitCloneCmd, let's verify its implementation to ensure it includes the directory existence check mentioned in the PR objectives.✅ Verification successful
GitCloneCmd implementation has proper directory existence check
The implementation in
common/teacmd/git.goalready includes the directory existence check before cloning:
- Verifies if target directory exists using
os.Stat()- Returns an error if directory already exists
- Only proceeds with git clone if directory check passes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GitCloneCmd implementation to verify directory existence check ast-grep --pattern 'func GitCloneCmd($_, $_, $_) { $$$ }'Length of output: 61
Script:
#!/bin/bash # Let's try a broader search for GitCloneCmd using ripgrep rg "GitCloneCmd" -A 10Length of output: 2263
cmd/world/root/root_test.go (1)
126-129: **** The previous review comment about potential cleanup implications and working directory restoration is still valid.
Merge activity
|
… command (#83) Closes: WORLD-1044 ## Overview if you world create with name == "foobar" and you already have a dir named "foobar", the error message you get isn't very helpful. ## Brief Changelog - add checking for the exists dir before doing clone ## Testing and Verifying - Manually tested using `world cardinal create` command - Adjusted unit test <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Enhanced error handling in the Git clone operation to prevent overwriting existing directories by returning an error if the target directory already exists. - **Tests** - Updated test cases to utilize a new directory structure for command execution, ensuring consistent behavior in command tests. - Modified the `TestBuild` function to clone repositories into a specific subdirectory within the test environment. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
zulkhair
force-pushed
the
daim/check_dir_exist_before_create_clone
branch
from
9e8086f
to
64feb1f
Compare
Closes: WORLD-1044
Overview
if you world create with name == "foobar" and you already have a dir named "foobar", the error message you get isn't very helpful.
Brief Changelog
Testing and Verifying
world cardinal createcommandSummary by CodeRabbit
TestBuildfunction to clone repositories into a specific subdirectory within the test environment.