chore: switch to version selector #18
Conversation
WalkthroughThis update modifies the command used in the policy creation API. The command-line flag specifying the version selector in the Changes
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: 0
🧹 Nitpick comments (4)
cmd/ctrlc/root/api/create/policy/policy.go (4)
196-196: Flag name updated, but variable name remains inconsistentThe flag has been renamed from
--deployment-version-selectorto--version-selector, which aligns with the PR objectives. However, the variable namedeploymentVersionSelectorhasn't been updated to match the new flag name. For better code clarity and maintainability, consider renaming the variable toversionSelectorto match the flag name.- var deploymentVersionSelector string + var versionSelector string // Later in the code: - cmd.Flags().StringVar(&deploymentVersionSelector, "version-selector", "", "JSON string for version selector") + cmd.Flags().StringVar(&versionSelector, "version-selector", "", "JSON string for version selector") // And in other places where the variable is used: - if deploymentVersionSelector != "" { + if versionSelector != "" { - if err := json.Unmarshal([]byte(deploymentVersionSelector), &selector); err != nil { + if err := json.Unmarshal([]byte(versionSelector), &selector); err != nil {
138-151: Update comment to match the flag name changeThe comment on line 138 still refers to "deployment version selector" but the flag has been updated to "version selector". For consistency, update the comment to match the new terminology.
- // Parse deployment version selector + // Parse version selector
39-39: Example uses old flag naming conventionThe example on line 39 shows usage of
--deployment-selector, which is consistent with the pattern of the newly renamed--version-selectorflag. Consider updating the usage examples to demonstrate the--version-selectorflag as well.- # Create a new policy with deployment selector - $ ctrlc create policy --name my-policy --workspace-id 00000000-0000-0000-0000-000000000000 --deployment-selector '{"type": "production"}' + # Create a new policy with deployment selector + $ ctrlc create policy --name my-policy --workspace-id 00000000-0000-0000-0000-000000000000 --deployment-selector '{"type": "production"}' + + # Create a new policy with version selector + $ ctrlc create policy --name my-policy --workspace-id 00000000-0000-0000-0000-000000000000 --version-selector '{"version": "1.0.0"}'
143-145: Update error message to match flag nameThe error message still references "deployment version selector" rather than the new flag name. Update it to maintain consistency with the renamed flag.
- return fmt.Errorf("invalid deployment version selector JSON: %w", err) + return fmt.Errorf("invalid version selector JSON: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/ctrlc/root/api/create/policy/policy.go(1 hunks)
🔇 Additional comments (1)
cmd/ctrlc/root/api/create/policy/policy.go (1)
147-151:❓ Verification inconclusive
Code works correctly despite terminology inconsistency
The parameter is correctly mapped to the API
DeploymentVersionSelectortype, so functionally this change works. However, the inconsistency between variable names and flag names may cause confusion for future maintainers.
🏁 Script executed:
#!/bin/bash # Check if there are any other references to "deployment-version-selector" or "deploymentVersionSelector" # in the codebase that might need updating for consistency echo "Searching for other references to deployment version selector..." rg --no-filename "deployment-version-selector" || echo "No other references to old flag name found" rg --no-filename "deploymentVersionSelector" -g "!policy.go" || echo "No other references to variable name found outside this file"Length of output: 624
Action: Standardize Naming for Clarity in DeploymentVersionSelector Usage
The functional mapping to the API’s
DeploymentVersionSelectoris correct; however, the naming inconsistency between the flag (deployment-version-selector) and the internal variable (DeploymentVersionSelector) might confuse future maintainers. Our search confirmed that the legacy flag name isn’t spread across the codebase, but the API’s naming is consistently used elsewhere. Consider aligning the flag and variable names or adding a clarifying comment if the divergence is intentional.
- File:
cmd/ctrlc/root/api/create/policy/policy.go(lines 147-151)- Snippet for Reference:
parsedDeploymentVersionSelector = &api.DeploymentVersionSelector{ DeploymentVersionSelector: selector, Name: name, }
Summary by CodeRabbit
--version-selectorwith an updated description for clearer usage.