fix: Gen cli for release refactor #15
Conversation
WalkthroughThis update adds new CLI commands for creating, updating, upserting, and deleting deployment versions and deployment version channels. It integrates the new commands into the existing command hierarchies using Cobra, adjusts flag shorthands for release commands, and introduces comprehensive API client support for these operations. Additionally, a new test file verifies backwards compatibility across the modified release and deployment functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CLI Command (Cobra)
participant A as API Client
participant S as API Server
U->>C: Execute command with flags
C->>A: Build request (create/update/upsert/delete)
A->>S: Send API request
S-->>A: Return response
A-->>C: Relay response
C-->>U: Output result
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 3
🧹 Nitpick comments (14)
cmd/ctrlc/root/api/delete/deploymentversionchannel/deployment-version-channel.go (1)
13-48: Consider marking required flags as required using the API.The implementation looks good, but I noticed that you're checking if
deploymentIDandnameare empty in theRunEfunction, returning an error if either is missing. For better UX, consider marking these flags as required using Cobra'sMarkFlagRequiredmethod.cmd.Flags().StringVar(&deploymentID, "deployment", "", "Deployment ID") cmd.Flags().StringVar(&name, "name", "", "Deployment version channel name") + + cmd.MarkFlagRequired("deployment") + cmd.MarkFlagRequired("name")This would make the CLI behavior consistent with other commands and provide better error messages to users before execution.
cmd/ctrlc/root/api/create/deploymentversionchannel/deployment-version-channel.go (2)
14-19: Consider grouping and validating related flags.
Currently, the flags (name,deploymentID,description,selector) are declared individually. If additional validation logic is needed later (e.g., ensuringselectorJSON aligns with specific schema), consider grouping them into a struct or a dedicated validation function to keep the flag-parsing code maintainable.
60-69: Add coverage for this command in automated tests.
The creation logic is fairly critical. Even a basic test validating a successful creation scenario and a failing scenario (e.g., invalid JSON inselector) would provide confidence in changes.Should we create an integration test that mocks the API call to verify correct handling of success/failure cases?
cmd/ctrlc/root/api/create/deploymentversion/deployment-version.go (3)
17-32: Avoid code duplication insafeConvertToDeploymentVersionStatus.
This helper appears in multiple files (both “create” and “upsert”). Consider embedding it in a shared package or utility file to follow DRY principles.
60-109: Ensure partial success handling for multiple deployments.
If the operation fails on any single deployment ID, the loop immediately returns an error, causing other deployments to remain un-upserted in that iteration. Confirm if this is the intended behavior, or if partial success with a summary of failed IDs is preferable.
115-129: Expand test coverage for multi-deployment usage.
Since the command allows specifying multiple deployment IDs, add or ensure test coverage for these cases (including partial success/failure scenarios if applicable) to avoid regressions.cmd/ctrlc/root/api/upsert/deploymentversion/deployment-version.go (2)
17-32: ConsolidatesafeConvertToDeploymentVersionStatuslogic.
This function is repeated in both “create” and “upsert” packages. To maintain consistency and reduce duplication, consider moving it to an internal shared utility.
91-108: Confirm partial success approach in multi-deployment scenario.
Similar to the “create” command, returning an error on a single failure means you might not upsert other IDs. Validate whether a partial success mechanism is needed if only one out of many fails.cmd/ctrlc/root/api/update/deploymentversion/deployment-version.go (3)
15-34: Simplify status conversion logicThe function correctly converts string status values to the appropriate enum type, but there are two issues:
- The
status := statassignment on line 19 is redundant and can be removed- The check for empty string on line 21 (
statusLower == "") is redundant since you already handle empty status on lines 16-18func safeConvertToDeploymentVersionStatus(stat string) (*api.UpdateDeploymentVersionJSONBodyStatus, error) { if stat == "" { return nil, nil } - status := stat - statusLower := strings.ToLower(status) - if statusLower == "ready" || statusLower == "" { + statusLower := strings.ToLower(stat) + if statusLower == "ready" { s := api.UpdateDeploymentVersionJSONBodyStatusReady return &s, nil } if statusLower == "building" { s := api.UpdateDeploymentVersionJSONBodyStatusBuilding return &s, nil } if statusLower == "failed" { s := api.UpdateDeploymentVersionJSONBodyStatusFailed return &s, nil } - return nil, fmt.Errorf("invalid deployment version status: %s", status) + return nil, fmt.Errorf("invalid deployment version status: %s", stat) }
72-78: Check for nil metadata before adding linksWhen adding links to metadata, ensure the metadata map is initialized to prevent potential nil pointer dereference.
if len(links) > 0 { linksJSON, err := json.Marshal(links) if err != nil { return fmt.Errorf("failed to marshal links: %w", err) } + if metadata == nil { + metadata = make(map[string]string) + } metadata["ctrlplane/links"] = string(linksJSON) }Although the metadata map is initialized through the flag on line 101, it's a good defensive programming practice to check for nil before accessing a map.
99-99: Consider changing the flag shorthand for deployment-version-idThe shorthand
-rfor deployment-version-id is somewhat unintuitive. Consider using-dor-iinstead, which might be more intuitive for users.test/backwards_compatibility_test.go (3)
12-18: Follow Go naming conventions for constantsGo constants typically use either
CamelCasefor exported constants orcamelCasefor unexported constants. The current snake_case naming doesn't follow Go conventions.const ( - WORKSPACE_ID = "5316df47-1f1c-4a5e-85e6-645e6b821616" - SYSTEM_ID = "55469883-36ae-450a-bc2b-60f6637ed3f4" - DEPLOYMENT_ID = "c585065c-4132-4b91-a479-cf45830b1576" - API_KEY = "f89b6f6172b99919.a02ec78ed6bb0729f860ca7bee5e44495b39eb543ed5c9d8dea7b05e55aa40bf" - API_URL = "http://localhost:3000/api" + WorkspaceID = "5316df47-1f1c-4a5e-85e6-645e6b821616" + SystemID = "55469883-36ae-450a-bc2b-60f6637ed3f4" + DeploymentID = "c585065c-4132-4b91-a479-cf45830b1576" + APIKey = "f89b6f6172b99919.a02ec78ed6bb0729f860ca7bee5e44495b39eb543ed5c9d8dea7b05e55aa40bf" + APIURL = "http://localhost:3000/api" )Also, consider using environment variables for sensitive values like API keys, even in tests.
48-68: Improve ID extraction method robustnessThe current ID extraction method is brittle and relies on string parsing. Consider using a proper JSON parser for more reliable extraction.
func extractID(output string) string { - lines := strings.Split(output, "\n") - for _, line := range lines { - if strings.Contains(line, "\"id\":") { - // Find the last occurrence of ":" to handle cases where the ID might contain colons - lastColon := strings.LastIndex(line, ":") - if lastColon != -1 { - // Get everything after the last colon - id := line[lastColon+1:] - // Remove any quotes, commas, and whitespace - id = strings.TrimSpace(id) - id = strings.Trim(id, ",") - id = strings.Trim(id, `"`) - id = strings.TrimSpace(id) - fmt.Println("ID:", id) - return id - } - } - } - return "" + var data map[string]interface{} + if err := json.Unmarshal([]byte(output), &data); err != nil { + // If not valid JSON, fall back to string parsing + lines := strings.Split(output, "\n") + for _, line := range lines { + if strings.Contains(line, "\"id\":") { + lastColon := strings.LastIndex(line, ":") + if lastColon != -1 { + id := line[lastColon+1:] + id = strings.TrimSpace(id) + id = strings.Trim(id, ",") + id = strings.Trim(id, `"`) + id = strings.TrimSpace(id) + fmt.Println("ID:", id) + return id + } + } + } + return "" + } + + // Extract ID from parsed JSON + if id, ok := data["id"].(string); ok { + fmt.Println("ID:", id) + return id + } + return "" }This improved version first tries to parse the output as JSON, and only falls back to string parsing if that fails.
1-257: Reduce console output in testsThe test contains many
fmt.Printlncalls that produce excessive output, making it harder to read test results. Consider usingt.Logort.Logfinstead, which only prints when tests fail or when running with verbose mode.Replace all instances of
fmt.Printlnwitht.Logort.Logf. For example:-fmt.Println("\n=== Testing Release Endpoints ===") +t.Log("\n=== Testing Release Endpoints ===")This makes the test output cleaner and follows standard Go testing practices.
🧰 Tools
🪛 golangci-lint (1.64.8)
178-178: Error return value is not checked
(errcheck)
253-253: Error return value is not checked
(errcheck)
254-254: Error return value is not checked
(errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/ctrlc/root/api/create/create.go(2 hunks)cmd/ctrlc/root/api/create/deploymentversion/deployment-version.go(1 hunks)cmd/ctrlc/root/api/create/deploymentversionchannel/deployment-version-channel.go(1 hunks)cmd/ctrlc/root/api/create/environment/environment.go(4 hunks)cmd/ctrlc/root/api/create/release/release.go(1 hunks)cmd/ctrlc/root/api/delete/delete.go(2 hunks)cmd/ctrlc/root/api/delete/deploymentversionchannel/deployment-version-channel.go(1 hunks)cmd/ctrlc/root/api/update/deploymentversion/deployment-version.go(1 hunks)cmd/ctrlc/root/api/update/update.go(2 hunks)cmd/ctrlc/root/api/upsert/deploymentversion/deployment-version.go(1 hunks)cmd/ctrlc/root/api/upsert/release/release.go(1 hunks)cmd/ctrlc/root/api/upsert/upsert.go(2 hunks)internal/api/client.gen.go(20 hunks)test/backwards_compatibility_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
cmd/ctrlc/root/api/update/update.go (1)
cmd/ctrlc/root/api/update/deploymentversion/deployment-version.go (1)
NewUpdateDeploymentVersionCmd(36-112)
cmd/ctrlc/root/api/upsert/upsert.go (1)
cmd/ctrlc/root/api/upsert/deploymentversion/deployment-version.go (1)
NewUpsertDeploymentVersionCmd(34-127)
cmd/ctrlc/root/api/delete/delete.go (1)
cmd/ctrlc/root/api/delete/deploymentversionchannel/deployment-version-channel.go (1)
NewDeleteDeploymentVersionChannelCmd(13-48)
cmd/ctrlc/root/api/create/deploymentversionchannel/deployment-version-channel.go (1)
internal/api/client.gen.go (1)
CreateDeploymentVersionChannelJSONRequestBody(597-597)
cmd/ctrlc/root/api/upsert/deploymentversion/deployment-version.go (1)
internal/api/client.gen.go (5)
UpsertDeploymentVersionJSONBodyStatus(388-388)UpsertDeploymentVersionJSONBodyStatusReady(76-76)UpsertDeploymentVersionJSONBodyStatusBuilding(74-74)UpsertDeploymentVersionJSONBodyStatusFailed(75-75)UpsertDeploymentVersionJSONRequestBody(600-600)
cmd/ctrlc/root/api/update/deploymentversion/deployment-version.go (1)
internal/api/client.gen.go (5)
UpdateDeploymentVersionJSONBodyStatus(404-404)UpdateDeploymentVersionJSONBodyStatusReady(83-83)UpdateDeploymentVersionJSONBodyStatusBuilding(81-81)UpdateDeploymentVersionJSONBodyStatusFailed(82-82)UpdateDeploymentVersionJSONRequestBody(603-603)
cmd/ctrlc/root/api/create/deploymentversion/deployment-version.go (1)
internal/api/client.gen.go (5)
UpsertDeploymentVersionJSONBodyStatus(388-388)UpsertDeploymentVersionJSONBodyStatusReady(76-76)UpsertDeploymentVersionJSONBodyStatusBuilding(74-74)UpsertDeploymentVersionJSONBodyStatusFailed(75-75)UpsertDeploymentVersionJSONRequestBody(600-600)
cmd/ctrlc/root/api/create/create.go (2)
cmd/ctrlc/root/api/create/deploymentversionchannel/deployment-version-channel.go (1)
NewCreateDeploymentVersionChannelCmd(14-69)cmd/ctrlc/root/api/create/deploymentversion/deployment-version.go (1)
NewCreateDeploymentVersionCmd(34-131)
🪛 golangci-lint (1.64.8)
test/backwards_compatibility_test.go
178-178: Error return value is not checked
(errcheck)
253-253: Error return value is not checked
(errcheck)
254-254: Error return value is not checked
(errcheck)
🔇 Additional comments (34)
cmd/ctrlc/root/api/create/release/release.go (1)
122-122:❓ Verification inconclusive
Flag shorthand change from
-tto-ris consistent but breakingThe shorthand for the
--created-atflag has been changed from-tto-r. This change promotes consistency with other deployment version commands that use-rfor similar timestamp flags, but it's a breaking change for users who might be using the-tshorthand in scripts or automation.
🏁 Script executed:
#!/bin/bash # Let's check if there are any documentation or examples that reference the old shorthand rg -A 5 "ctrlc.+create release.+\-t" --glob "*.md" --glob "*.txt"Length of output: 67
Attention: Breaking Change in '--created-at' Flag Shorthand
The shorthand for the
--created-atflag has been updated from-tto-rfor consistency with related commands. An automated search of repository documentation (Markdown and text files) did not reveal any references using the old-tshorthand. However, be aware that this remains a breaking change for any external automation or scripts that might still use-t.
- Location:
cmd/ctrlc/root/api/create/release/release.go(Line 122)- Action: Please double-check any internal or external integrations to ensure that they have been updated to use the new shorthand
-r.cmd/ctrlc/root/api/upsert/upsert.go (1)
4-4: New deployment version command enhances CLI capabilitiesThe addition of the deploymentversion package import and the new
deployment-versionupsert command adds valuable functionality to the CLI for managing deployment versions. This follows the same pattern as existing commands, making it intuitive for users.Also applies to: 22-22
cmd/ctrlc/root/api/upsert/release/release.go (1)
118-118:❓ Verification inconclusive
Flag shorthand change from
-tto-ris consistent but breakingSimilar to the change in the create command, the shorthand for the
--created-atflag has been changed from-tto-r. This change makes the CLI more consistent but could break existing scripts or workflows that use the-tshorthand.
🏁 Script executed:
#!/bin/bash # Let's check if there are any documentation or examples that reference the old shorthand rg -A 5 "ctrlc.+upsert release.+\-t" --glob "*.md" --glob "*.txt"Length of output: 67
CLI Flag Shorthand Update—Check External Scripts for Breakage
The
--created-atflag in the upsert command now uses-rinstead of the previous-t(seecmd/ctrlc/root/api/upsert/release/release.go:118). Our initial search of in-repository documentation and examples (in markdown and text files) revealed no references to the old-tshorthand. However, because this change resembles the one made in the create command, there’s still a risk that external scripts or workflows might be relying on-t.• File Impact:
- File:
cmd/ctrlc/root/api/upsert/release/release.go- Line: 118
Please ensure that any external automation, documentation, or user scripts referencing
-tare updated accordingly.cmd/ctrlc/root/api/update/update.go (1)
4-4: New deployment version update command enhances CLI capabilitiesThe addition of the deploymentversion package import and the new
deployment-versionupdate command expands the CLI's functionality for managing deployment versions. This is well-integrated with the existing command structure and follows established patterns.Also applies to: 21-21
cmd/ctrlc/root/api/delete/delete.go (2)
4-4: LGTM! New import for deployment version channel deletion.The import for the deployment version channel deletion handler is correctly added.
24-24: LGTM! Command added for deleting deployment version channels.The new command for deleting deployment version channels is properly integrated into the command hierarchy, following the same pattern as other delete commands.
cmd/ctrlc/root/api/create/create.go (2)
4-5: LGTM! New imports for deployment version commands.The imports for the deployment version and deployment version channel creation are correctly added.
25-26: LGTM! New commands for deployment version functionality.The new commands for creating deployment version channels and deployment versions are properly integrated into the command hierarchy.
cmd/ctrlc/root/api/create/environment/environment.go (4)
17-17: LGTM! Added deployment version channels variable.The new variable for storing deployment version channels is properly defined.
29-31: LGTM! Updated example with deployment version channels.The example is clear and demonstrates how to use both release channels and deployment version channels.
46-46: LGTM! Assigning deployment version channels to request body.The deployment version channels are correctly assigned to the request body.
70-70: LGTM! Added flag for deployment version channels.The flag for specifying deployment version channels is properly defined with a clear description and appropriate shorthand
-d.cmd/ctrlc/root/api/create/deploymentversionchannel/deployment-version-channel.go (2)
31-45: Handle potential security or parsing issues in selector JSON.
You are already returning an error if parsing fails. This is good. However, consider whether there's any need to validate thatversionSelectorconforms to an expected schema or data type, as unvalidated JSON can lead to confusion if downstream components expect specific structures.Would you like to implement deeper JSON schema validation on
selector?
46-57: Check for nil or invalid pointer usage.
Sincedescriptionis passed by reference inDescription: &description,, ensure that the default empty string does not lead to unexpected behavior if the API expects a non-nil pointer with content. In practice, this should be fine, but keep an eye on edge cases where an empty string might produce a different effect from a nil pointer on the API side.cmd/ctrlc/root/api/create/deploymentversion/deployment-version.go (1)
37-44: Validate combined usage of multiple flags (metadata, config, etc.).
When multiple maps (metadata, config, links) merge into the final structure, potential key collisions or overshadowing might occur (for example,metadata["ctrlplane/links"]). If that is intentional, please confirm; otherwise, consider clarifying what happens with overlapping keys.cmd/ctrlc/root/api/upsert/deploymentversion/deployment-version.go (2)
60-66: Preserve consistent error messages.
Ensure that your error messages (e.g., “failed to create API client”) align across thecreateandupsertcommands. This helps unify the CLI user experience and debugging.
76-82: Guard against nil map assignment.
Ifmetadatais empty and you dometadata["ctrlplane/links"] = …, it still works since you’ve initialized the map. Just confirm that the newly introduced “ctrlplane/links” key does not overwrite any existing property.internal/api/client.gen.go (17)
72-84: Well-defined status constants for deployment versionsThe new constants for
UpsertDeploymentVersionJSONBodyStatusandUpdateDeploymentVersionJSONBodyStatusfollow the same pattern as the existing status constants for releases, providing a consistent approach to status management.
125-136: LGTM: DeploymentVersion struct is well-definedThe
DeploymentVersionstruct is properly structured with all necessary fields and follows the same pattern as other entity types likeRelease. The fields are appropriately typed, and optional fields are correctly marked as pointers.
186-190: Appropriate integration of DeploymentVersion in JobWithTriggerThe addition of the
DeploymentVersionfield to theJobWithTriggerstruct is a logical extension that provides consistency with the existingReleasefield, allowing jobs to reference deployment versions.
366-389: Well-structured request body types for deployment version operationsThe new JSON body types for deployment version operations are well-structured and follow the same patterns as existing request body types. Required and optional fields are appropriately defined.
390-405: Consistent request body definition for updating deployment versionsThe
UpdateDeploymentVersionJSONBodystruct and its associated status type follow the same pattern as other update operations, making all fields optional to allow partial updates.
438-439: Good addition of DeploymentVersionChannels to environment creationAdding the
DeploymentVersionChannelsfield to theCreateEnvironmentJSONBodystruct provides proper integration between environments and deployment version channels.
596-604: Consistent request body type definitionsThe request body type definitions for deployment version operations follow the consistent pattern used throughout the codebase.
990-1004: Complete client interface methods for deployment versionsThe client interface is properly updated with methods for all deployment version operations, maintaining consistency with other API operations.
1171-1242: Well-implemented client methods for deployment version operationsThe implementation of client methods for deployment version operations follows the same pattern as other client methods, ensuring consistent error handling and request building.
1315-1325: Proper implementation of DeleteDeploymentVersionChannelThe implementation of the
DeleteDeploymentVersionChannelmethod follows the same pattern as other delete operations, ensuring consistent behavior across the API.
1949-2075: Consistent URL creation functions for new endpointsThe new functions for creating URLs for deployment version operations are well-implemented and follow the same patterns as existing functions, ensuring consistent URL construction.
3697-3711: Complete ClientWithResponsesInterface updateThe
ClientWithResponsesInterfaceis properly updated with methods for all deployment version operations, maintaining consistency with other API operations.
3891-3979: Well-defined response types for deployment version operationsThe response types for deployment version operations are comprehensive, including all necessary HTTP status codes and properly structured JSON response bodies.
5156-5205: Consistent implementation of ClientWithResponses methodsThe implementation of
ClientWithResponsesmethods for deployment version operations follows the same pattern as other methods, ensuring consistent response handling.
5731-5861: Thorough response parsing for deployment version operationsThe response parsing functions for deployment version operations handle all expected status codes and properly unmarshal JSON responses into the appropriate types.
5259-6084: Proper implementation of DeleteDeploymentVersionChannel with responsesThe implementation of the
DeleteDeploymentVersionChannelWithResponsemethod and its associated response parsing function follow the same pattern as other delete operations with responses.
2009-2011:❓ Verification inconclusive
Verify API endpoint paths for consistency
The deployment version endpoints use
/v1/deployment-versionsand/v1/deployment-versions/{id}patterns, which are consistent with other entity endpoints. However, it's worth verifying that these paths match the API specification and any documentation.Also applies to: 2056-2058
🏁 Script executed:
#!/bin/bash # Check for other API endpoints to confirm consistency echo "Checking API endpoint path patterns in the codebase:" rg -A 1 "operationPath :=" internal/api/client.gen.go | grep -v "deployment-version" | grep "\(v1\|api/v1\)" | head -n 10Length of output: 883
Action Required: Verify API Endpoints Against Documentation
The deployment version endpoints (
/v1/deployment-versionsand/v1/deployment-versions/{id}) align with the versioning observed in other endpoints (e.g.,/v1/deployments,/v1/environments,/v1/job-agents/name). However, note that one endpoint uses a slightly different prefix (/api/v1/cloud-locations/%s), so please confirm with the API specifications and related documentation that this variation is intentional.
- Verify that all endpoint paths in
internal/api/client.gen.go(specifically around lines 2009–2011 and 2056–2058) truly conform to the expected API design.- Double-check the discrepancy between the
/v1/and/api/v1/prefixes to ensure consistency across the API.
Summary by CodeRabbit
New Features
Tests