feat: Implemented deployment status check #88
Conversation
Closes: WORLD-1309
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
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 (
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/world/forge/deployment.go (4)
242-248: Refactor If-Else Chain to Switch StatementConsider refactoring the if-else chain into a switch statement for better readability and maintainability.
Refactored code:
switch { case cardinalOK: fmt.Print("OK\n") case cardinalResultCode == 0: fmt.Printf("FAIL %s\n", statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) default: fmt.Printf("FAIL %d %s\n", cardinalResultCode, statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) }🧰 Tools
🪛 golangci-lint (1.62.2)
242-242: ifElseChain: rewrite if-else to switch statement
(gocritic)
250-256: Refactor If-Else Chain to Switch StatementSimilar to the previous suggestion, refactor the if-else chain for
nakamaOKinto a switch statement.Refactored code:
switch { case nakamaOK: fmt.Print("OK\n") case nakamaResultCode == 0: fmt.Printf("FAIL %s\n", statusFailRegEx.ReplaceAllString(nakamaResultStr, "")) default: fmt.Printf("FAIL %d %s\n", nakamaResultCode, statusFailRegEx.ReplaceAllString(nakamaResultStr, "")) }🧰 Tools
🪛 golangci-lint (1.62.2)
250-250: ifElseChain: rewrite if-else to switch statement
(gocritic)
258-259: Format Comments CorrectlyEnsure there's a space after the
//in comment lines to follow Go's formatting conventions.-//fmt.Println() +// fmt.Println() -//fmt.Println(string(result)) +// fmt.Println(string(result))🧰 Tools
🪛 golangci-lint (1.62.2)
258-258: commentFormatting: put a space between
//and comment text(gocritic)
137-262: Refactor 'status' Function to Improve MaintainabilityThe
statusfunction contains 87 statements, exceeding the recommended limit of 50 statements. Breaking it into smaller helper functions would enhance readability and maintainability.Consider refactoring:
- Extract data fetching and parsing into separate functions.
- Separate the logic for printing deployment details and health checks.
🧰 Tools
🪛 golangci-lint (1.62.2)
165-165: Error return value is not checked
(errcheck)
182-182: Error return value is not checked
(errcheck)
183-183: Error return value is not checked
(errcheck)
187-187: Error return value is not checked
(errcheck)
188-188: Error return value is not checked
(errcheck)
192-192: Error return value is not checked
(errcheck)
212-212: Error return value is not checked
(errcheck)
220-220: Error return value is not checked
(errcheck)
137-137: Function 'status' has too many statements (87 > 50)
(funlen)
258-258: commentFormatting: put a space between
//and comment text(gocritic)
242-242: ifElseChain: rewrite if-else to switch statement
(gocritic)
250-250: ifElseChain: rewrite if-else to switch statement
(gocritic)
🪛 GitHub Check: Go
[failure] 165-165:
Error return value is not checked (errcheck)
[failure] 182-182:
Error return value is not checked (errcheck)
[failure] 192-192:
Error return value is not checked (errcheck)
[failure] 212-212:
Error return value is not checked (errcheck)
[failure] 220-220:
Error return value is not checked (errcheck)
[failure] 221-221:
Error return value is not checked (errcheck)
[failure] 223-223:
Error return value is not checked (errcheck)
[failure] 224-224:
Error return value is not checked (errcheck)
[failure] 225-225:
Error return value is not checked (errcheck)
[failure] 227-227:
Error return value is not checked (errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/world/forge/deployment.go(2 hunks)cmd/world/forge/forge.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/world/forge/deployment.go
165-165: Error return value is not checked
(errcheck)
182-182: Error return value is not checked
(errcheck)
183-183: Error return value is not checked
(errcheck)
187-187: Error return value is not checked
(errcheck)
188-188: Error return value is not checked
(errcheck)
192-192: Error return value is not checked
(errcheck)
212-212: Error return value is not checked
(errcheck)
220-220: Error return value is not checked
(errcheck)
137-137: Function 'status' has too many statements (87 > 50)
(funlen)
258-258: commentFormatting: put a space between // and comment text
(gocritic)
242-242: ifElseChain: rewrite if-else to switch statement
(gocritic)
250-250: ifElseChain: rewrite if-else to switch statement
(gocritic)
🪛 GitHub Check: Go
cmd/world/forge/deployment.go
[failure] 165-165:
Error return value is not checked (errcheck)
[failure] 182-182:
Error return value is not checked (errcheck)
[failure] 192-192:
Error return value is not checked (errcheck)
[failure] 212-212:
Error return value is not checked (errcheck)
[failure] 220-220:
Error return value is not checked (errcheck)
[failure] 221-221:
Error return value is not checked (errcheck)
[failure] 223-223:
Error return value is not checked (errcheck)
[failure] 224-224:
Error return value is not checked (errcheck)
[failure] 225-225:
Error return value is not checked (errcheck)
[failure] 227-227:
Error return value is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit & Coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/world/forge/forge.go (3)
14-14: Verify Change of Local Base URL PortThe local base URL port has been changed from
8081to8001. Ensure that the local development server is configured to run on port8001and that this change is intentional.
177-186: Addition of 'status' Command Looks GoodThe
statusCmdcommand is correctly defined and integrated into the deployment commands. It follows the consistent pattern used by other commands.
220-221: Register 'status' Command with Deployment CommandsThe
statusCmdis properly added to thedeploymentCmdand the base command. This ensures thestatuscommand is available to users.
- fixed all lint issues - error handling for all JSON response parsing - switched to using port 8001 instead of 8081 - got rid of magic number for project name length
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
cmd/world/forge/forge_test.go (2)
37-37: Consider using dynamic port allocation for test server.Instead of using a fixed port (8001) which could cause conflicts in CI environments or when running tests in parallel, consider letting the system assign an available port automatically.
- listener, err := net.Listen("tcp", ":8001") + listener, err := net.Listen("tcp", ":0") // Let the system choose an available port
Line range hint
41-83: Consider refactoring the test server handler for better maintainability.The current implementation could be improved in several ways:
- Split the large handler function into separate handlers for each endpoint
- Standardize error responses across all endpoints
- Add handling for common HTTP methods (PUT, PATCH, DELETE) where applicable
Example refactor:
+func (s *ForgeTestSuite) handleAPI(w http.ResponseWriter, r *http.Request) { + handlers := map[string]http.HandlerFunc{ + "/api/organization": s.handleOrganizationList, + "/api/organization/empty-org-id": s.handleOrganizationGet, + "/api/organization/test-org-id": s.handleOrganizationGet, + "/api/organization/test-org-id/project": s.handleProjectList, + "/api/organization/test-org-id/project/test-project-id": s.handleProjectGet, + // ... other routes + } + + if handler, exists := handlers[r.URL.Path]; exists { + handler(w, r) + return + } + + s.writeErrorResponse(w, "Not found", http.StatusNotFound) +} + +func (s *ForgeTestSuite) writeErrorResponse(w http.ResponseWriter, message string, status int) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + json.NewEncoder(w).Encode(map[string]string{"error": message}) +}cmd/world/forge/deployment.go (5)
17-17: Consider adding documentation for the regex pattern.Add a comment explaining what characters the regex pattern is filtering out and why.
+// statusFailRegEx filters out special characters from status messages, keeping only alphanumeric characters, dots, and spaces var statusFailRegEx = regexp.MustCompile(`[^a-zA-Z0-9\. ]+`)
137-138: Consider breaking down the function for better maintainability.While the nolint directives are explained, the function's complexity suggests it could benefit from being split into smaller, focused functions:
validateDeploymentDataprocessBuildInfoprocessHealthCheck
229-248: Add retry mechanism for health checks.The health check could fail temporarily due to network issues or services starting up. Consider implementing a retry mechanism with backoff.
+const ( + maxRetries = 3 + retryDelay = time.Second * 2 +) + func status(ctx context.Context) error { // ... existing code ... healthURL := fmt.Sprintf("%s/api/health/%s", baseURL, projectID) - result, err = sendRequest(ctx, http.MethodGet, healthURL, nil) + var result []byte + for i := 0; i < maxRetries; i++ { + result, err = sendRequest(ctx, http.MethodGet, healthURL, nil) + if err == nil { + break + } + if i < maxRetries-1 { + time.Sleep(retryDelay * time.Duration(i+1)) + } + } if err != nil { return eris.Wrap(err, "Failed to get health") }
313-322: Consider using a helper function for status output formatting.The status output formatting logic is duplicated for Cardinal and Nakama services. Extract this into a helper function to reduce code duplication.
+func formatServiceStatus(serviceName, host string, ok bool, resultCode int, resultStr string) string { + var status strings.Builder + status.WriteString(fmt.Sprintf("\t%s: %s - ", serviceName, host)) + switch { + case ok: + status.WriteString("OK") + case resultCode == 0: + status.WriteString(fmt.Sprintf("FAIL %s", statusFailRegEx.ReplaceAllString(resultStr, ""))) + default: + status.WriteString(fmt.Sprintf("FAIL %d %s", resultCode, statusFailRegEx.ReplaceAllString(resultStr, ""))) + } + return status.String() +}Then use it in the main function:
- fmt.Printf("\tCardinal: %s - ", cardinalHost) - switch { - case cardinalOK: - fmt.Print("OK\n") - case cardinalResultCode == 0: - fmt.Printf("FAIL %s\n", statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) - default: - fmt.Printf("FAIL %d %s\n", cardinalResultCode, statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) - } - fmt.Printf("\tNakama: %s - ", nakamaHost) - switch { - case nakamaOK: - fmt.Print("OK\n") - case nakamaResultCode == 0: - fmt.Printf("FAIL %s\n", statusFailRegEx.ReplaceAllString(nakamaResultStr, "")) - default: - fmt.Printf("FAIL %d %s\n", nakamaResultCode, statusFailRegEx.ReplaceAllString(nakamaResultStr, "")) - } + fmt.Println(formatServiceStatus("Cardinal", cardinalHost, cardinalOK, cardinalResultCode, cardinalResultStr)) + fmt.Println(formatServiceStatus("Nakama", nakamaHost, nakamaOK, nakamaResultCode, nakamaResultStr))Also applies to: 323-331
226-227: Remove commented-out debug code.Remove the commented-out debug print statements as they add noise to the codebase.
- // fmt.Println() - // fmt.Println(string(result)) - // fmt.Println() - // fmt.Println(string(result))Also applies to: 333-334
cmd/world/forge/project.go (1)
192-193: Use constant in error message for consistency.While the validation correctly uses
MaxProjectNameLen, the error message still has a hardcoded "50". This could become misleading if the constant value changes.- fmt.Printf("Error: Project name cannot be longer than 50 characters\n") + fmt.Printf("Error: Project name cannot be longer than %d characters\n", MaxProjectNameLen)
📜 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_test.go(1 hunks)cmd/world/forge/project.go(2 hunks)
🔇 Additional comments (4)
cmd/world/forge/forge_test.go (2)
Line range hint
4-9: Well-structured test suite with comprehensive coverage!The test implementation follows best practices:
- Uses table-driven tests for thorough coverage
- Proper setup and teardown methods
- Good mocking of external dependencies
- Clear test case descriptions
Also applies to: 26-29
Line range hint
1-1: Add test coverage for deployment status check functionality.The PR implements deployment status check functionality, but corresponding test cases are missing. Consider adding test cases to cover:
- Successful status check scenarios
- Various deployment states
- Error cases (e.g., invalid project, network errors)
- Edge cases (e.g., partial deployments)
Would you like me to help generate the test cases for the status check functionality?
cmd/world/forge/deployment.go (1)
165-174: Use safer type assertions consistently.Previous review comments highlighted the need for safer type assertions. While some improvements have been made, ensure consistent usage throughout the function.
cmd/world/forge/project.go (1)
15-15: LGTM! Good use of a named constant.Extracting the magic number into a well-named constant improves maintainability and reusability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
==========================================
- Coverage 50.48% 50.34% -0.15%
==========================================
Files 63 63
Lines 4431 4658 +227
==========================================
+ Hits 2237 2345 +108
- Misses 1904 1987 +83
- Partials 290 326 +36 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/world/forge/forge_test.go (1)
209-223: Consider adding error cases to the mock handlers.The handlers are well-implemented but could benefit from additional test coverage:
- Add cases for invalid HTTP methods
- Add cases for malformed request parameters
- Include error scenarios in the mock responses
Also applies to: 225-248, 250-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/world/forge/forge_test.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
cmd/world/forge/forge_test.go
300-300: Error return value of w.Write is not checked
(errcheck)
🪛 GitHub Check: Go
cmd/world/forge/forge_test.go
[failure] 300-300:
Error return value of w.Write is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit & Coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/world/forge/forge_test.go (3)
37-37: LGTM: Port number update is consistent.The change from port 8081 to 8001 aligns with the corresponding update in the production code.
74-79: LGTM: Well-structured handler routes.The new routes for deployment status and health checks are properly integrated into the test server configuration.
578-649: LGTM: Comprehensive test coverage for status command.The test cases effectively cover:
- Success scenarios for deployed and undeployed projects
- Error handling for invalid configurations
- Edge cases for missing organization and project IDs
- handle write error in tests (make lint happy) - handle "destroy" and "reset" deployment types
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
cmd/world/forge/deployment.go (3)
17-17: Consider making the regex pattern more specific.The current pattern removes all characters except alphanumeric, dots, and spaces. Consider if you need to preserve other meaningful characters in error messages, such as hyphens or parentheses.
-var statusFailRegEx = regexp.MustCompile(`[^a-zA-Z0-9\. ]+`) +var statusFailRegEx = regexp.MustCompile(`[^a-zA-Z0-9\-\.\(\) ]+`)
315-322: Refactor status check logic into a helper function.The status check logic is duplicated between Cardinal and Nakama services. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+func formatServiceStatus(ok bool, resultCode int, resultStr string) string { + switch { + case ok: + return "OK" + case resultCode == 0: + return fmt.Sprintf("FAIL %s", statusFailRegEx.ReplaceAllString(resultStr, "")) + default: + return fmt.Sprintf("FAIL %d %s", resultCode, statusFailRegEx.ReplaceAllString(resultStr, "")) + } +} // In the main function: -switch { -case cardinalOK: - fmt.Print("OK\n") -case cardinalResultCode == 0: - fmt.Printf("FAIL %s\n", statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) -default: - fmt.Printf("FAIL %d %s\n", cardinalResultCode, statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) -} +fmt.Printf("%s\n", formatServiceStatus(cardinalOK, cardinalResultCode, cardinalResultStr))
187-189: Define deployment types as constants.The deployment types should be defined as constants to prevent typos and improve maintainability.
+const ( + DeploymentTypeDeploy = "deploy" + DeploymentTypeDestroy = "destroy" + DeploymentTypeReset = "reset" +) -if data["type"] != "deploy" && data["type"] != "destroy" && data["type"] != "reset" { +deploymentType, ok := data["type"].(string) +if !ok { + return eris.New("Failed to unmarshal deployment type") +} +switch deploymentType { +case DeploymentTypeDeploy, DeploymentTypeDestroy, DeploymentTypeReset: + // Valid type +default: + return eris.Errorf("Unknown deployment type %s", deploymentType) +}cmd/world/forge/forge_test.go (2)
209-223: Add test cases for error scenarios in handleStatusDeployed.The test handler provides only the happy path. Consider adding test cases for malformed data and error scenarios.
func (s *ForgeTestSuite) handleStatusDeployed(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) return } + // Add query parameter to simulate different scenarios + if r.URL.Query().Get("error") == "malformed" { + s.writeJSONString(w, `{"data": "not_an_object"}`) + return + } s.writeJSONString(w, `{"data":{ "project_id":"test-project-id", "type":"deploy", "executor_id":"test-executor-id", "execution_time":"2001-01-01T01:02:00Z", "build_number":1, "build_time":"2001-01-01T01:01:00Z", "build_state":"finished" }}`) }
37-37: Document the port number change.The test server port was changed from 8081 to 8001. Add a comment explaining why this change was made to prevent future confusion.
- listener, err := net.Listen("tcp", ":8001") + // Use port 8001 to match the worldForgeBaseURLLocal constant in forge.go + listener, err := net.Listen("tcp", ":8001")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/world/forge/deployment.go(2 hunks)cmd/world/forge/forge_test.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit & Coverage
🔇 Additional comments (1)
cmd/world/forge/deployment.go (1)
165-174:⚠️ Potential issueImprove type assertion safety for deployment data.
The type assertion for the
datafield could be more robust. While there is a nil check, the type assertion could still panic if the data is not of the expected type.-if response["data"] != nil { - // data = null is returned when there are no deployments, so we have to check for that before we - // try to cast the response into a json object map, since this is not an error but the cast would - // fail - var ok bool - data, ok = response["data"].(map[string]any) - if !ok { - return eris.New("Failed to unmarshal deployment data") - } -} +dataAny, exists := response["data"] +if !exists { + return eris.New("Missing 'data' field in response") +} +if dataAny != nil { + data, ok = dataAny.(map[string]any) + if !ok { + return eris.Errorf("Invalid type for 'data' field: %T", dataAny) + } +}Likely invalid or redundant comment.
- handling deploy types: deploy, destroy, and reset - use "passed" instead of "finished" for deploy build state - show health if destroy failed - always show health for resets - changed build_time to build_start_time to fix bug
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/world/forge/deployment.go (1)
165-174:⚠️ Potential issueHandle type assertions safely.
Multiple type assertions are performed without proper error handling. Use the two-value form of type assertions to handle potential errors gracefully.
Apply this pattern:
-if response["data"] != nil { - data = response["data"].(map[string]any) +if response["data"] != nil { + var ok bool + data, ok = response["data"].(map[string]any) + if !ok { + return eris.New("Failed to unmarshal deployment data") + } }cmd/world/forge/forge_test.go (2)
298-302:⚠️ Potential issueCheck error from Write operation.
The error from
w.Writeshould be checked.func (s *ForgeTestSuite) writeJSONString(w http.ResponseWriter, data string) { w.Header().Set("Content-Type", "application/json") - _, err := w.Write([]byte(data)) - s.Require().NoError(err) + if _, err := w.Write([]byte(data)); err != nil { + s.T().Fatalf("Failed to write response: %v", err) + } }
579-650: 🛠️ Refactor suggestionEnhance test coverage for deployment types and build states.
Add test cases for:
- Different deployment types (deploy, destroy, reset)
- Different build states (passed, failed, running)
- Various health check responses
Add these test cases:
{ name: "Success - Reset deployment type", config: globalconfig.GlobalConfig{ ProjectID: "test-project-id-reset", Credential: globalconfig.Credential{ Token: "test-token", }, }, expectedError: false, }, { name: "Success - In-progress build", config: globalconfig.GlobalConfig{ ProjectID: "test-project-id-building", Credential: globalconfig.Credential{ Token: "test-token", }, }, expectedError: false, },
🧹 Nitpick comments (3)
cmd/world/forge/deployment.go (3)
137-152: Consider breaking down the status function for better maintainability.The function is marked with multiple nolint directives for complexity. Consider extracting the following into separate functions:
- Deployment status check
- Build status processing
- Health check processing
This would improve readability and make the code easier to maintain.
223-243: Consider using an enum or constants for deployment types and build states.The deployment types and build states are currently hardcoded strings. Consider defining them as constants to prevent typos and improve maintainability.
+const ( + DeploymentTypeDeploy = "deploy" + DeploymentTypeDestroy = "destroy" + DeploymentTypeReset = "reset" + + BuildStatePassed = "passed" + BuildStateFailed = "failed" +)
271-352: Extract health check processing into a separate function.The health check processing logic is complex and would benefit from being extracted into a dedicated function for better maintainability.
Consider creating a separate function:
func processInstanceHealth(instance map[string]any) error { // Extract and validate instance details // Process Cardinal health // Process Nakama health return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/world/forge/deployment.go(2 hunks)cmd/world/forge/forge_test.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit & Coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/world/forge/deployment.go (1)
17-17: LGTM! Well-defined regex pattern.The regex pattern effectively filters unwanted characters while preserving dots and spaces for readable error messages.
cmd/world/forge/forge_test.go (2)
209-223: LGTM! Well-structured test handler.The test handler provides a good mock response for deployment status testing.
37-37: LGTM! Port change is consistent.The test server port change to 8001 aligns with the implementation.
- plus some minor code cleanup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cmd/world/forge/deployment.go (5)
17-17: Add documentation for the regex pattern.Add a comment explaining the purpose of this regex pattern and what characters it's designed to filter out.
+// statusFailRegEx filters out special characters from health check error messages, +// keeping only alphanumeric characters, periods, and spaces. var statusFailRegEx = regexp.MustCompile(`[^a-zA-Z0-9\. ]+`)
137-137: Consider breaking down the complex function.The function has multiple linter suppressions for complexity. Consider breaking it down into smaller, focused functions for better maintainability:
- One for deployment status checking
- One for health status checking
- One for instance details processing
154-154: Consider using url.URL for robust URL construction.Instead of using string formatting, consider using the
url.URLtype for more robust URL construction that handles escaping correctly.+import "net/url" + +func buildStatusURL(baseURL, projectID string) (string, error) { + u, err := url.Parse(baseURL) + if err != nil { + return "", err + } + u.Path = fmt.Sprintf("/api/deployment/%s", projectID) + return u.String(), nil +}
246-247: Remove commented debug code.Remove the commented-out debug print statements as they're not needed in production code.
Also applies to: 353-354
339-342: Consider standardizing error message format.The error message format differs between the cases:
FAIL messageFAIL code messageConsider standardizing to always include the code (use 0 explicitly) for consistency:
- fmt.Printf("FAIL %s\n", statusFailRegEx.ReplaceAllString(cardinalResultStr, "")) + fmt.Printf("FAIL %d %s\n", 0, statusFailRegEx.ReplaceAllString(cardinalResultStr, ""))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/world/forge/deployment.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Go
- GitHub Check: Unit & Coverage
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
cmd/world/forge/deployment.go (2)
165-174: Well-implemented type assertion handling!Good job implementing safe type assertions with proper error handling and clear comments explaining the null data case.
191-193: Deployment types properly implemented.The code correctly implements the requested "destroy" and "reset" deployment types as mentioned in the previous review.
Closes: WORLD-1309
Summary by CodeRabbit
New Features
Improvements
Bug Fixes