feat: policy create cmd #17
Conversation
WalkthroughThis update introduces a new policy creation command to the CLI and extends the underlying API client's capabilities. In the command module, a new policy command is registered and implemented with comprehensive flag handling, JSON parsing, and error reporting. The API client now includes several new types and methods for handling policies and resource schemas, such as the creation and deletion of these entities, thereby expanding the functionality of the client interface. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant PolicyCmd
participant APIClient
participant Server
User->>CLI: Executes "create policy" command
CLI->>PolicyCmd: Parse flags and JSON inputs
PolicyCmd->>APIClient: Call CreatePolicy with request body
APIClient->>Server: POST /policy request
Server-->>APIClient: Return response (success/error)
APIClient-->>PolicyCmd: Pass response data
PolicyCmd-->>CLI: Output formatted result
CLI-->>User: Display policy creation status
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: 2
🧹 Nitpick comments (6)
cmd/ctrlc/root/api/create/policy/policy.go (2)
58-84: Refactor duplicate JSON parsing code.There's repetitive code for parsing the three selector types which could be extracted into a helper function to reduce duplication.
+func parseJSONSelector(jsonStr string, selectorName string) (*map[string]interface{}, error) { + if jsonStr == "" { + return nil, nil + } + var parsedSelector map[string]interface{} + if err := json.Unmarshal([]byte(jsonStr), &parsedSelector); err != nil { + return nil, fmt.Errorf("invalid %s selector JSON: %w", selectorName, err) + } + return &parsedSelector, nil +} // Parse selectors from JSON strings -var deploymentSelector *map[string]interface{} -if deploymentTargetSelector != "" { - var parsedSelector map[string]interface{} - if err := json.Unmarshal([]byte(deploymentTargetSelector), &parsedSelector); err != nil { - return fmt.Errorf("invalid deployment target selector JSON: %w", err) - } - deploymentSelector = &parsedSelector +deploymentSelector, err := parseJSONSelector(deploymentTargetSelector, "deployment target") +if err != nil { + return err } -var environmentSelector *map[string]interface{} -if environmentTargetSelector != "" { - var parsedSelector map[string]interface{} - if err := json.Unmarshal([]byte(environmentTargetSelector), &parsedSelector); err != nil { - return fmt.Errorf("invalid environment target selector JSON: %w", err) - } - environmentSelector = &parsedSelector +environmentSelector, err := parseJSONSelector(environmentTargetSelector, "environment target") +if err != nil { + return err } -var resourceSelector *map[string]interface{} -if resourceTargetSelector != "" { - var parsedSelector map[string]interface{} - if err := json.Unmarshal([]byte(resourceTargetSelector), &parsedSelector); err != nil { - return fmt.Errorf("invalid resource target selector JSON: %w", err) - } - resourceSelector = &parsedSelector +resourceSelector, err := parseJSONSelector(resourceTargetSelector, "resource target") +if err != nil { + return err }
98-110: Consider consistent handling of empty arrays.You're initializing
parsedVersionUserApprovalsandparsedVersionRoleApprovalsto empty arrays when empty, but not doing the same forparsedVersionAnyApprovals. This inconsistency could lead to confusion.// Parse version any approvals var parsedVersionAnyApprovals *[]struct { RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` } if versionAnyApprovals != "" { var approvals []struct { RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` } if err := json.Unmarshal([]byte(versionAnyApprovals), &approvals); err != nil { return fmt.Errorf("invalid version any approvals JSON: %w", err) } parsedVersionAnyApprovals = &approvals +} else { + empty := []struct { + RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` + }{} + parsedVersionAnyApprovals = &empty }internal/api/client.gen.go (4)
144-149: Consider renaming the nested field.
The struct fieldDeploymentVersionSelectorwithinDeploymentVersionSelectormay lead to slight confusion.
272-287: Rename the struct to avoid confusion.
The namePolicy1is not descriptive. Consider a more meaningful name for clarity.
388-393: Confirm role ID usage.
Consider documenting or validating whetherroleIdcan be empty.
4709-4733: Consider using HTTP 201 for policy creation.
The structure referencesPolicy1; ensure naming consistency to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctrlc/root/api/create/create.go(2 hunks)cmd/ctrlc/root/api/create/policy/policy.go(1 hunks)internal/api/client.gen.go(22 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
cmd/ctrlc/root/api/create/policy/policy.go (4)
internal/cliutil/config.go (1)
GetString(9-21)internal/api/client.go (1)
NewAPIKeyClientWithResponses(10-19)internal/api/client.gen.go (3)
VersionUserApproval(395-397)DeploymentVersionSelector(145-149)PolicyTarget(290-294)internal/cliutil/output.go (1)
HandleResponseOutput(16-30)
cmd/ctrlc/root/api/create/create.go (1)
cmd/ctrlc/root/api/create/policy/policy.go (1)
NewCreatePolicyCmd(15-203)
🔇 Additional comments (35)
cmd/ctrlc/root/api/create/create.go (2)
7-7: LGTM: Import added for the new policy package.The import for the policy package has been correctly added.
32-32: LGTM: Policy command registration properly added.The
policy.NewCreatePolicyCmd()has been correctly added to the list of create subcommands.cmd/ctrlc/root/api/create/policy/policy.go (6)
15-29: LGTM: Well-structured variable declarations.The variable declarations for command flags are well-organized and comprehensive.
30-49: LGTM: Command metadata and examples well defined.The command is properly configured with appropriate usage description and helpful examples that cover various use cases.
153-172: LGTM: Request body construction is correct.The construction of the policy request body looks correct and includes all the necessary fields.
183-196: LGTM: Flag definitions are comprehensive.All necessary flags are defined with descriptive help text and appropriate shorthand options where applicable.
198-200: LGTM: Required flags are properly marked.The required flags (
nameandworkspace-id) are correctly marked as required.
138-151:✅ Verification successful
Unusual pattern for handling DeploymentVersionSelector.
Unlike other selectors, the deployment version selector embeds the policy name. Please verify this is the intended behavior and consistent with the API expectations.
🏁 Script executed:
#!/bin/bash # Check if other commands use the same pattern for deployment version selectors echo "Checking how other commands use DeploymentVersionSelector..." rg -A 5 "DeploymentVersionSelector" --type go | grep -v "policy.go"Length of output: 2091
DeploymentVersionSelector Pattern Confirmed
After verification, the embedding of the policy name into the DeploymentVersionSelector is consistent with its usage in the autogenerated API client code (see
internal/api/client.gen.go). The approach matches the defined API expectations, and no further changes are necessary.internal/api/client.gen.go (27)
112-117: All good.
This block is auto-generated code. No obvious issues found.
289-294: All good.
No issues found in thePolicyTargetdefinition.
383-387: Confirm runtime validation for approval count.
If a certain minimum or non-zero threshold is required, ensure it's enforced elsewhere.
394-398: Consider userId validation.
Ensure invalid or emptyuserIdis handled appropriately.
619-632: All good.
No issues found. Ensure that upstream validation confirms valid arguments for the fields.
713-715: All good.
This alias is consistent with typical code generation patterns.
734-736: All good.
New request body alias looks fine.
1162-1165: API interface extension looks fine.
No concerns about these new methods.
1197-1200: API interface extension for resource schema creation is fine.
All good here.
1202-1203: New delete method is fine.
No issues found.
1607-1618: Implementation for CreatePolicyWithBody is standard codegen.
No issues found.
1619-1629: Implementation for CreatePolicy is standard codegen.
No issues found.
1775-1786: Implementation for CreateResourceSchemaWithBody is standard codegen.
No issues found.
1787-1798: Implementation for CreateResourceSchema is standard codegen.
No issues found.
1799-1810: Implementation for DeleteResourceSchema is standard codegen.
No issues found.
4057-4060: Interface methods for creating policy with response.
Looks good.
4092-4095: Interface methods for CreateResourceSchemaWithResponse.
All good.
4096-4098: New interface method to delete resource schema.
Implementation looks straightforward.
4917-4950: New response structure for resource schema creation.
Looks consistent with typical 201 creation semantics.
4952-4977: New response structure for resource schema deletion.
No issues found.
5777-5793: ClientWithResponses methods for creating policy.
No issues found.
5896-5904: Method for CreateResourceSchemaWithBodyWithResponse.
All good.
5905-5912: Method for CreateResourceSchemaWithResponse.
All good.
5913-5921: Method for DeleteResourceSchemaWithResponse.
All good.
6896-6930: ParseCreatePolicyResponse handles 200 instead of 201.
Verify if the server should return 201 for successful policy creation.
7207-7257: Resource schema creation parse logic is consistent with a 201 response.
No concerns.
7258-7294: Resource schema deletion parse logic is standard.
No issues found.
| Targets: []api.PolicyTarget{ | ||
| { | ||
| DeploymentSelector: deploymentSelector, | ||
| EnvironmentSelector: environmentSelector, | ||
| ResourceSelector: resourceSelector, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider supporting multiple targets.
The API model appears to support multiple targets (using a slice), but the implementation only creates a single target. If multiple targets should be supported, consider adding flags or accepting a JSON array for all targets.
🏁 Script executed:
#!/bin/bash
# Check if policies can have multiple targets
echo "Checking if the API supports multiple targets..."
rg -A 15 "PolicyTarget" --type go | grep -B 2 -A 2 "Targets.*\[\]"Length of output: 1149
Action: Enhance support for multiple policy targets
The current implementation in cmd/ctrlc/root/api/create/policy/policy.go creates a single target:
Targets: []api.PolicyTarget{
{
DeploymentSelector: deploymentSelector,
EnvironmentSelector: environmentSelector,
ResourceSelector: resourceSelector,
},
},However, as confirmed in internal/api/client.gen.go, the API model supports multiple targets via a slice. If the design intent is to allow multiple policy targets, please consider extending the CLI to either add additional flags or allow a JSON array input so that users can specify more than one target.
| // CreatePolicyJSONBody defines parameters for CreatePolicy. | ||
| type CreatePolicyJSONBody struct { | ||
| DenyWindows []struct { | ||
| Dtend *time.Time `json:"dtend,omitempty"` | ||
| Rrule *map[string]interface{} `json:"rrule,omitempty"` | ||
| TimeZone string `json:"timeZone"` | ||
| } `json:"denyWindows"` | ||
| DeploymentVersionSelector *DeploymentVersionSelector `json:"deploymentVersionSelector,omitempty"` | ||
| Description *string `json:"description,omitempty"` | ||
| Enabled *bool `json:"enabled,omitempty"` | ||
| Name string `json:"name"` | ||
| Priority *float32 `json:"priority,omitempty"` | ||
| Targets []PolicyTarget `json:"targets"` | ||
| VersionAnyApprovals *[]struct { | ||
| RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` | ||
| } `json:"versionAnyApprovals,omitempty"` | ||
| VersionRoleApprovals []struct { | ||
| RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` | ||
| RoleId *string `json:"roleId,omitempty"` | ||
| } `json:"versionRoleApprovals"` | ||
| VersionUserApprovals []VersionUserApproval `json:"versionUserApprovals"` | ||
| WorkspaceId string `json:"workspaceId"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify inline struct with DenyWindow.
denyWindows duplicates the DenyWindow struct fields. Consider reusing that struct to reduce duplication.
Summary by CodeRabbit