feat: add deployment reset command #89
Conversation
How to use the Graphite Merge QueueAdd the label graphite/merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughThe pull request introduces a significant refactoring of deployment functionality across multiple files in the Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant DeploymentService
User->>CLI: Execute deployment command
CLI->>DeploymentService: deployment(context, deployType)
alt deployType is "deploy"
DeploymentService-->>CLI: Deploy project
else deployType is "destroy"
DeploymentService-->>CLI: Destroy project
else deployType is "reset"
DeploymentService-->>CLI: Reset project
end
CLI-->>User: Display operation result
Possibly related PRs
Suggested reviewers
Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
cmd/world/forge/deployment.go (3)
14-14: Fix the function comment for clarityThe function comment has a typo and is unclear. It currently reads "// Deployment a project". Consider revising it for better readability.
Suggestion:
-// Deployment a project +// Deploys a project based on the given deployment type
53-56: Enhance error handling with specific contextWhile wrapping the error provides context, it might be beneficial to include the HTTP status code or response body if available. This can help diagnose issues more effectively when a deployment fails.
Consider modifying the error handling to capture more details:
if err != nil { // If 'sendRequest' returns detailed error information, include it here return eris.Wrapf(err, "Failed to %s project: %v", deployType, err) }
59-60: Improve output message formattingTo enhance the user experience, consider formatting the command in the output message as code for better readability.
Apply this change:
-fmt.Println(" $ 'world forge deployment status'") +fmt.Println(" $ world forge deployment status")Alternatively, you can use backticks or color coding if your CLI supports it.
cmd/world/forge/project.go (1)
190-192: Define 'maxNameLength' as a constant for reusabilityConsider defining
maxNameLengthas a package-level constant to improve maintainability and reuse across functions if needed.Suggestion:
At the top of the file, add:
const maxNameLength = 50Then in the function:
-maxNameLength := 50cmd/world/forge/forge_test.go (2)
239-245: Consider standardizing error responses across handlers.The implementation is correct, but there's an opportunity to improve error handling consistency.
Consider extracting the method check and error response to a shared helper function:
+func methodNotAllowed(w http.ResponseWriter) { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) +} + func (s *ForgeTestSuite) handleReset(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { - http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + methodNotAllowed(w) return } s.writeJSON(w, map[string]interface{}{"data": "reset started"}) }
616-694: Enhance reset test coverage.While the test implementation is thorough, consider these improvements:
- Add a test case for cancelled reset operation (input "n")
- Make test case descriptions more specific (e.g., "Success - Reset with valid credentials and confirmation")
- Consider consolidating similar error cases
Example test case to add:
+ { + name: "Success - Reset cancelled by user", + config: globalconfig.GlobalConfig{ + OrganizationID: "test-org-id", + ProjectID: "test-project-id", + Credential: globalconfig.Credential{ + Token: "test-token", + }, + }, + input: "n", + expectedError: false, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/world/forge/deployment.go(2 hunks)cmd/world/forge/forge.go(3 hunks)cmd/world/forge/forge_test.go(4 hunks)cmd/world/forge/project.go(1 hunks)
🔇 Additional comments (7)
cmd/world/forge/forge.go (4)
162-162: Consolidated deployment functions improve maintainabilityRefactoring to use the unified
deploymentfunction enhances code reuse and simplifies maintenance.
173-173: Consolidated deployment functions improve maintainabilityThe
destroycommand now correctly callsdeploymentwith"destroy", aligning with the refactored design.
177-184: 'reset' command implementation is consistent and correctThe new
resetcommand is added appropriately and follows the existing pattern for deployment commands.
220-220: Registered 'reset' command successfullyThe
resetCmdis correctly added todeploymentCmd, ensuring the command is available in the CLI.cmd/world/forge/forge_test.go (3)
64-65: LGTM! Reset endpoint handler is properly integrated.The new reset endpoint follows the established API path pattern and is correctly wired to its handler.
606-614: LGTM! TestDestroy properly adapted to use deployment function.The change maintains test coverage while aligning with the new unified deployment approach.
518-518: Verify deployment type constant usage.The change to use deployment() with "deploy" type is correct. However, we should verify if there's a constant defined for deployment types.
✅ Verification successful
String literal "deploy" usage is consistent
The usage of "deploy" as a string literal is consistent across both the test file and its implementation in forge.go. No constants are defined or used for deployment types in the codebase, indicating this is the intended pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for deployment type constants rg -g '*.go' -B 2 -A 2 'const.*[Dd]eploy[Tt]ype'Length of output: 50
Script:
#!/bin/bash # Search for "deploy" string usage in function calls rg -g '*.go' 'deployment\(.*"deploy".*\)' # Search for deployment type definitions or enums ast-grep --pattern 'type $_ struct { $$$ Type string $$$ }'Length of output: 734
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
- Coverage 50.48% 50.00% -0.49%
==========================================
Files 63 63
Lines 4431 4612 +181
==========================================
+ Hits 2237 2306 +69
- Misses 1904 1984 +80
- Partials 290 322 +32 ☔ View full report in Codecov by Sentry. |
zulkhair
force-pushed
the
daim/add_deployment_reset_command
branch
from
d44c4b3 to
b2ac2f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/world/forge/deployment.go (1)
19-20: Consider improving function documentation.The function comment "Deployment a project" is grammatically incorrect and lacks details about the possible deployment types.
Apply this diff to improve the documentation:
-// Deployment a project +// deployment executes a deployment operation (deploy/destroy/reset) on a project. +// deployType must be one of: "deploy", "destroy", "reset"cmd/world/forge/forge_test.go (1)
Line range hint
866-950: Consider adding test cases for invalid deployment type.The test suite should verify that the
deploymentfunction properly handles invalid deployment types.Add a test case for invalid deployment type:
func (s *ForgeTestSuite) TestReset() { testCases := []struct { name string config globalconfig.GlobalConfig input string expectedError bool }{ + { + name: "Error - Invalid deployment type", + config: globalconfig.GlobalConfig{ + OrganizationID: "test-org-id", + ProjectID: "test-project-id", + Credential: globalconfig.Credential{ + Token: "test-token", + }, + }, + input: "y", + expectedError: true, + }, // ... existing test cases ... } for _, tc := range testCases { s.Run(tc.name, func() { err := globalconfig.SaveGlobalConfig(tc.config) s.Require().NoError(err) getInput = func() (string, error) { return tc.input, nil } defer func() { getInput = originalGetInput }() - err = deployment(s.ctx, "reset") + deployType := "reset" + if tc.name == "Error - Invalid deployment type" { + deployType = "invalid" + } + err = deployment(s.ctx, deployType) if tc.expectedError { s.Require().Error(err) } else { s.Require().NoError(err) } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/world/forge/deployment.go(2 hunks)cmd/world/forge/forge.go(3 hunks)cmd/world/forge/forge_test.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit & Coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
cmd/world/forge/forge.go (2)
162-162: Consistent refactoring of deployment commands.The deployment and destroy commands have been refactored to use the unified
deploymentfunction, which improves code maintainability by centralizing deployment logic.Also applies to: 173-174
232-232: LGTM!The reset command is properly added to the deployment command group.
cmd/world/forge/deployment.go (1)
65-66: LGTM!The status messages are properly updated to use the dynamic
deployType.cmd/world/forge/forge_test.go (3)
64-65: LGTM!The reset endpoint is properly added to the test server handler.
380-386: LGTM!The reset handler follows the same pattern as other handlers, properly checking the HTTP method and returning appropriate responses.
665-665: LGTM!The deployment test cases are properly updated to use the new
deploymentfunction with appropriate deployment types.Also applies to: 856-861
Merge activity
|
Closes: WORLD-1327 & WORLD-1328
## Overview
Check destroy command and add reset command
## Brief Changelog
- merge the deploy, destroy func
## Testing and Verifying
Manual test runing `world forge deployment reset` command
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
## Release Notes
- **New Features**
- Added a new project reset functionality
- Enhanced deployment commands with more flexible action types
- **Improvements**
- Consolidated deployment-related actions into a single method
- Improved error handling and context for deployment operations
- **Changes**
- Updated deployment command structure to support deploy, destroy, and reset actions
- Standardized deployment method signature across different operations
The release introduces more flexible project management capabilities with a unified deployment approach.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
zulkhair
force-pushed
the
daim/add_deployment_reset_command
branch
from
b2ac2f6 to
ea0a524
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/world/forge/deployment.go (1)
59-62:⚠️ Potential issueValidate deployType parameter before making API call.
The function should validate the deployType parameter to prevent invalid API calls.
🧹 Nitpick comments (2)
cmd/world/forge/deployment.go (1)
19-20: Enhance function documentation.The docstring should be more descriptive to help users understand the function's purpose and usage.
-// Deployment a project +// Deployment manages project deployment operations based on the specified type. +// +// Parameters: +// - ctx: The context for the operation +// - deployType: The type of deployment operation ("deploy", "destroy", or "reset") +// +// Returns an error if the operation fails.cmd/world/forge/forge_test.go (1)
866-944: Add test case for user cancellation.The test suite should include a case where the user cancels the reset operation by entering "n" when prompted, similar to the destroy test cases.
func (s *ForgeTestSuite) TestReset() { testCases := []struct { name string config globalconfig.GlobalConfig input string expectedError bool }{ + { + name: "Success - Cancelled reset", + config: globalconfig.GlobalConfig{ + OrganizationID: "test-org-id", + ProjectID: "test-project-id", + Credential: globalconfig.Credential{ + Token: "test-token", + }, + }, + input: "n", + expectedError: false, + }, // ... existing test cases ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/world/forge/deployment.go(2 hunks)cmd/world/forge/forge.go(3 hunks)cmd/world/forge/forge_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/world/forge/forge.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Go
- GitHub Check: Go
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
cmd/world/forge/deployment.go (1)
65-66: LGTM! Clear and helpful output messages.The messages are well-formatted, consistent, and provide useful next steps for the user.
cmd/world/forge/forge_test.go (3)
64-65: LGTM! Reset handler properly registered.The reset handler follows the same pattern as other deployment handlers.
380-386: LGTM! Consistent handler implementation.The handleReset function follows the same pattern as other handlers and maintains consistency in:
- HTTP method validation
- Response format
- Error handling
856-864: LGTM! Test updated correctly.The test has been properly updated to use the new deployment function while maintaining the same test coverage.
Closes: WORLD-1327 & WORLD-1328
Overview
Check destroy command and add reset command
Brief Changelog
Testing and Verifying
Manual test runing
world forge deployment resetcommandSummary by CodeRabbit
Release Notes
New Features
Improvements
Changes
The release introduces more flexible project management capabilities with a unified deployment approach.