Conversation
WalkthroughThis change introduces policy management to the application. It adds new data structures for policies, their targets, deny windows, and approval requirements. The apply command is extended to process and upsert policies via an API client, with concurrent handling for efficiency. Supporting API client types are updated for compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplyCmd
participant PolicyProcessor
participant APIClient
User->>ApplyCmd: runApply()
ApplyCmd->>PolicyProcessor: processAllPolicies(policies)
loop for each Policy
PolicyProcessor->>APIClient: upsertPolicy(requestBody)
APIClient-->>PolicyProcessor: Policy ID / Error
end
PolicyProcessor-->>ApplyCmd: All policies processed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🧹 Nitpick comments (3)
cmd/ctrlc/root/apply/policy.go (3)
58-75: Simplify deny windows conversion with embedded struct definitionThe code defines an anonymous struct inside the function, which makes maintenance more difficult. Consider defining the target struct type at package level or importing it if it's already defined elsewhere.
+ // DenyWindowRequest represents the API request format for deny windows + type DenyWindowRequest struct { + Dtend *time.Time `json:"dtend,omitempty"` + Rrule *map[string]interface{} `json:"rrule,omitempty"` + TimeZone string `json:"timeZone"` + } func createPolicyRequestBody(policy Policy) api.UpsertPolicyJSONRequestBody { // ... // Convert deny windows - denyWindows := make([]struct { - Dtend *time.Time `json:"dtend,omitempty"` - Rrule *map[string]interface{} `json:"rrule,omitempty"` - TimeZone string `json:"timeZone"` - }, len(policy.DenyWindows)) + denyWindows := make([]DenyWindowRequest, len(policy.DenyWindows)) // ...
92-105: Simplify version role approvals conversion with predefined structSimilar to the deny windows issue, this code defines an anonymous struct inline, which complicates the code and makes it harder to maintain.
+ // VersionRoleApprovalRequest represents the API request format for role approvals + type VersionRoleApprovalRequest struct { + RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` + RoleId string `json:"roleId"` + } func createPolicyRequestBody(policy Policy) api.UpsertPolicyJSONRequestBody { // ... - versionRoleApprovals := make([]struct { - RequiredApprovalsCount *float32 `json:"requiredApprovalsCount,omitempty"` - RoleId string `json:"roleId"` - }, len(policy.VersionRoleApprovals)) + versionRoleApprovals := make([]VersionRoleApprovalRequest, len(policy.VersionRoleApprovals)) // ...
23-28: Consider adding concurrency limitingThe current implementation launches a goroutine for each policy without a limit, which could lead to resource exhaustion if there are many policies.
Consider using a worker pool pattern or semaphore to limit concurrency:
func processAllPolicies( ctx context.Context, client *api.ClientWithResponses, policies []Policy, ) { if len(policies) == 0 { return } + // Limit concurrency to a reasonable number + maxConcurrency := 5 + sem := make(chan struct{}, maxConcurrency) + var wg sync.WaitGroup for _, policy := range policies { wg.Add(1) - go processPolicy(ctx, client, policy, &wg) + go func(policy Policy) { + sem <- struct{}{} // Acquire semaphore + defer func() { <-sem }() // Release semaphore + processPolicy(ctx, client, policy, &wg) + }(policy) } wg.Wait() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
cmd/ctrlc/root/apply/cmd.go(1 hunks)cmd/ctrlc/root/apply/policy.go(1 hunks)cmd/ctrlc/root/apply/types.go(2 hunks)internal/api/client.gen.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/ctrlc/root/apply/policy.go (2)
internal/api/client.gen.go (6)
Policy(428-461)UpsertPolicyJSONRequestBody(1080-1080)PolicyTarget(490-494)VersionAnyApproval(707-709)VersionUserApproval(718-720)DeploymentVersionSelector(293-297)cmd/ctrlc/root/apply/types.go (5)
Policy(107-119)PolicyTarget(121-125)VersionAnyApproval(139-141)VersionUserApproval(143-145)DeploymentVersionSelector(133-137)
cmd/ctrlc/root/apply/types.go (2)
internal/api/client.gen.go (8)
System(638-653)Policy(428-461)PolicyTarget(490-494)DenyWindow(176-180)DeploymentVersionSelector(293-297)VersionAnyApproval(707-709)VersionUserApproval(718-720)VersionRoleApproval(712-715)internal/api/resource_provider.go (1)
ResourceProvider(52-57)
internal/api/client.gen.go (1)
cmd/ctrlc/root/apply/types.go (1)
VersionAnyApproval(139-141)
🔇 Additional comments (10)
cmd/ctrlc/root/apply/cmd.go (1)
48-48: LGTM: Policy processing successfully integrated into apply workflowThe integration of the policy processing step follows a consistent pattern with the existing resource handling flow. This is a clean addition to the apply command that enables the new policy management feature.
cmd/ctrlc/root/apply/policy.go (1)
14-29: LGTM: Concurrent policy processing implementationThe
processAllPoliciesfunction provides an efficient approach to process multiple policies concurrently using goroutines and WaitGroups, which follows similar patterns used elsewhere in the codebase.internal/api/client.gen.go (2)
483-483: Confirm data model change implicationsThe type of
VersionAnyApprovalshas been changed from what appears to be a pointer to a slice (*[]VersionAnyApproval) to a pointer to a single struct (*VersionAnyApproval). This change means policies can now have only one "any approval" configuration instead of potentially multiple.Please verify that this change is backward compatible with existing data and that you've handled the migration for any policies that might have had multiple "any approval" configurations.
876-877: API contract change for policy JSON structureThe changes to both
VersionAnyApprovalsandVersionRoleApprovalsfields in theUpsertPolicyJSONBodystruct modify the API request structure. This aligns with theVersionAnyApprovalstruct defined incmd/ctrlc/root/apply/types.go.Since this is a generated file (
client.gen.go), ensure these changes are made to the source API specification rather than directly editing this file. Also, confirm that any API clients expecting the previous structure have been updated accordingly.cmd/ctrlc/root/apply/types.go (6)
3-4: LGTM: Time package importThe import of the "time" package is needed to support the new
DtEndfield of type*time.Timein theDenyWindowstruct.
10-10: LGTM: Config extended with Policies fieldThe
Configstruct is properly extended with an optionalPoliciesfield, correctly using theomitemptytag to make it optional in the YAML representation.
121-125: LGTM: PolicyTarget implementationThe
PolicyTargetstruct correctly defines optional selector fields with appropriate YAML tags and matches the corresponding API definition structure.
133-137: LGTM: DeploymentVersionSelector implementationThe
DeploymentVersionSelectorstruct correctly defines the required fields with appropriate YAML tags and matches the API definition structure, though the field order differs slightly from the API struct.
139-150: LGTM: Approval type structs implementationThe approval type structs (
VersionAnyApproval,VersionUserApproval, andVersionRoleApproval) correctly define the required fields with appropriate YAML tags and match the corresponding API definition structures.
106-119:Details
❌ Incorrect review comment
Verify Policy struct field completeness
The
Policystruct contains essential fields for policy configuration but appears to have fewer fields compared to the API definition ininternal/api/client.gen.go. The API struct includes additional fields likeConcurrencyLimit,MinimumReleaseInterval,ReleaseSequencing, etc.Please confirm if this difference is intentional and how these missing fields are handled when data is sent to the API.
🏁 Script executed:
#!/bin/bash # Description: Check how missing Policy fields are handled when mapping to API requests # Find the policy processing function to see how Policy structs are mapped to API requests rg -A 5 -B 5 "func.*process.*[Pp]olic" cmd/Length of output: 1971
🏁 Script executed:
#!/bin/bash # Display the implementation of processPolicy to inspect field mapping rg -A50 -B2 "func processPolicy" cmd/ctrlc/root/apply/policy.goLength of output: 1708
🏁 Script executed:
#!/bin/bash # Extract full createPolicyRequestBody implementation rg -A200 -B0 "func createPolicyRequestBody" cmd/ctrlc/root/apply/policy.goLength of output: 3715
🏁 Script executed:
#!/bin/bash # Inspect the API UpsertPolicyJSONRequestBody struct for additional fields rg -A20 "type UpsertPolicyJSONRequestBody" -n .Length of output: 2059
🏁 Script executed:
#!/bin/bash # Extract the UpsertPolicyJSONBody definition to inspect all fields rg -A50 "type UpsertPolicyJSONBody" internal/api/client.gen.goLength of output: 2921
No missing Policy fields—API definition matches
After inspectingcreatePolicyRequestBodyand theUpsertPolicyJSONBodyininternal/api/client.gen.go, thePolicystruct covers every field the API expects. There are noConcurrencyLimit,MinimumReleaseInterval, orReleaseSequencingfields in the generated client, so the original suggestion doesn’t apply.Likely an incorrect or invalid review comment.
| func processPolicy( | ||
| ctx context.Context, | ||
| client *api.ClientWithResponses, | ||
| policy Policy, | ||
| policyWg *sync.WaitGroup, | ||
| ) { | ||
| defer policyWg.Done() | ||
|
|
||
| body := createPolicyRequestBody(policy) | ||
| if _, err := upsertPolicy(ctx, client, body); err != nil { | ||
| log.Error("Failed to create policy", "name", policy.Name, "error", err) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling needs improvement
The error from policy creation is logged but not propagated, which means the apply command will succeed even if policy creation fails. This differs from how other resource processing errors might be handled.
Consider collecting and returning errors like other processing functions might do, or at least propagating a non-zero exit code to indicate some operations failed:
func processAllPolicies(
ctx context.Context,
client *api.ClientWithResponses,
policies []Policy,
-) {
+) []error {
if len(policies) == 0 {
- return
+ return nil
}
+ var errMu sync.Mutex
+ var errs []error
var wg sync.WaitGroup
for _, policy := range policies {
wg.Add(1)
- go processPolicy(ctx, client, policy, &wg)
+ go func(policy Policy) {
+ defer wg.Done()
+ if err := processPolicy(ctx, client, policy); err != nil {
+ errMu.Lock()
+ errs = append(errs, err)
+ errMu.Unlock()
+ }
+ }(policy)
}
wg.Wait()
+ return errs
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/ctrlc/root/apply/policy.go around lines 31 to 44, the function
processPolicy logs errors when policy creation fails but does not propagate
them, causing the apply command to succeed incorrectly. Modify the function to
return an error instead of void, propagate the error up to the caller, and
ensure the caller handles it by collecting errors or setting a non-zero exit
code to indicate failure.
There was a problem hiding this comment.
this is how we are doing it in the other apply files
There was a problem hiding this comment.
🧩 Analysis chain
@adityachoudhari26 You're absolutely right. After checking the other apply files, I can confirm that your approach to error handling (logging errors but not propagating them) is consistent with the established pattern in the codebase. The other processAll* functions follow the same pattern of logging errors at the point they occur but continuing with other operations.
This approach allows the CLI to attempt processing all resources even if some fail, rather than stopping at the first error. I'll make note of this pattern for future reviews.
🏁 Script executed:
#!/bin/bash
# Find processAll* functions in other apply files and check error handling patterns
echo "=== processAll* functions in other files ==="
grep -r "func processAll" cmd/ctrlc/root/apply/ --include="*.go" | grep -v "policy.go"
echo -e "\n=== Error handling in system.go ==="
if [ -f cmd/ctrlc/root/apply/system.go ]; then
cat cmd/ctrlc/root/apply/system.go | grep -A 10 -B 5 "log.Error"
fi
echo -e "\n=== Error handling in resource.go ==="
if [ -f cmd/ctrlc/root/apply/resource.go ]; then
cat cmd/ctrlc/root/apply/resource.go | grep -A 10 -B 5 "log.Error"
fi
echo -e "\n=== Error handling in relationship.go ==="
if [ -f cmd/ctrlc/root/apply/relationship.go ]; then
cat cmd/ctrlc/root/apply/relationship.go | grep -A 10 -B 5 "log.Error"
fi
echo -e "\n=== How runApply handles these functions ==="
grep -A 3 "processAll" cmd/ctrlc/root/apply/cmd.go
Length of output: 2967
@adityachoudhari26 You're absolutely right. After checking the other apply files, I can confirm that your approach to error handling (logging errors but not propagating them) is consistent with the established pattern in the codebase.
I see the same pattern in system.go and resource.go where errors are logged but not returned:
// From system.go
if err != nil {
log.Error("Failed to upsert system", "name", system.Name, "error", err)
return
}
// From resource.go
if err != nil {
log.Error("Failed to upsert resources", "name", provider.Name, "error", err)
return
}And in cmd.go, all these functions are called sequentially without error handling:
processAllSystems(ctx, client, workspaceID, config.Systems)
processResourceProvider(ctx, client, workspaceID.String(), config.Providers)
processResourceRelationships(ctx, client, workspaceID.String(), config.Relationships)
processAllPolicies(ctx, client, config.Policies)Your implementation maintains consistency with the existing patterns in the codebase. Sorry for the confusion in my earlier suggestion.
🧬 Code Graph Analysis Results
Relevant snippets from cmd/ctrlc/root/apply/types.go (lines 107-125, 133-145):
Policy struct {
Name string `yaml:"name"`
Description *string `yaml:"description,omitempty"`
Priority *float32 `yaml:"priority,omitempty"`
Enabled *bool `yaml:"enabled,omitempty"`
WorkspaceId string `yaml:"workspaceId"`
Targets []PolicyTarget `yaml:"targets"`
DenyWindows []DenyWindow `yaml:"denyWindows,omitempty"`
DeploymentVersionSelector *DeploymentVersionSelector `yaml:"deploymentVersionSelector,omitempty"`
VersionAnyApprovals *VersionAnyApproval `yaml:"versionAnyApprovals,omitempty"`
VersionUserApprovals []VersionUserApproval `yaml:"versionUserApprovals,omitempty"`
VersionRoleApprovals []VersionRoleApproval `yaml:"versionRoleApprovals,omitempty"`
}
PolicyTarget struct {
DeploymentSelector *map[string]any `yaml:"deploymentSelector,omitempty"`
EnvironmentSelector *map[string]any `yaml:"environmentSelector,omitempty"`
ResourceSelector *map[string]any `yaml:"resourceSelector,omitempty"`
}
DeploymentVersionSelector struct {
Name string `yaml:"name"`
DeploymentVersionSelector map[string]any `yaml:"deploymentVersionSelector"`
Description *string `yaml:"description,omitempty"`
}
VersionAnyApproval struct {
RequiredApprovalsCount float32 `yaml:"requiredApprovalsCount"`
}
VersionUserApproval struct {
UserId string `yaml:"userId"`
}Relevant snippets from internal/api/client.gen.go (lines 490-494, 293-297, 707-720, 1080):
PolicyTarget struct {
DeploymentSelector *map[string]interface{} `json:"deploymentSelector"`
EnvironmentSelector *map[string]interface{} `json:"environmentSelector"`
ResourceSelector *map[string]interface{} `json:"resourceSelector"`
}
DeploymentVersionSelector struct {
DeploymentVersionSelector map[string]interface{} `json:"deploymentVersionSelector"`
Description *string `json:"description,omitempty"`
Name string `json:"name"`
}
VersionAnyApproval struct {
RequiredApprovalsCount float32 `json:"requiredApprovalsCount"`
}
VersionUserApproval struct {
UserId string `json:"userId"`
}
UpsertPolicyJSONRequestBody UpsertPolicyJSONBodySummary of internal/cliutil/config.go snippet (lines 9-21):
// GetString attempts to retrieve a string value for a given flag from a cobra.Command.
// It first checks the command flags, then environment variables if the flag is not set.
// Parameters:
// - cmd: *cobra.Command - the command to check flags on
// - flag: string - the name of the flag to retrieve
// Returns:
// - string: the value of the flag or environment variable, or empty string if none found
func GetString(cmd *cobra.Command, flag string) string {
value, _ := cmd.Flags().GetString(flag)
if value != "" {
return value
}
value, _ = cmd.Flags().GetString(flag)
if value != "" {
return value
}
return os.Getenv(flag)
}These snippets provide the data structures and API client types used in policy.go for processing policies, including the Policy struct and related nested structs, as well as the API request body struct UpsertPolicyJSONRequestBody. The config utility snippet shows how configuration values might be retrieved, which could be relevant if the "other apply files" use a similar pattern for configuration or flag handling.
| func upsertPolicy( | ||
| ctx context.Context, | ||
| client *api.ClientWithResponses, | ||
| policy api.UpsertPolicyJSONRequestBody, | ||
| ) (string, error) { | ||
| resp, err := client.UpsertPolicyWithResponse(ctx, policy) | ||
|
|
||
| if err != nil { | ||
| return "", fmt.Errorf("API request failed: %w", err) | ||
| } | ||
|
|
||
| if resp.StatusCode() >= 400 { | ||
| return "", fmt.Errorf("API returned error status: %d", resp.StatusCode()) | ||
| } | ||
|
|
||
| if resp.JSON200 != nil { | ||
| return resp.JSON200.Id.String(), nil | ||
| } | ||
|
|
||
| return "", fmt.Errorf("unexpected response format") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error details from API response
The error handling in upsertPolicy could be improved by including error details from the API response when available, rather than just returning the status code.
if resp.StatusCode() >= 400 {
- return "", fmt.Errorf("API returned error status: %d", resp.StatusCode())
+ errorMsg := "Unknown error"
+ if resp.HTTPResponse != nil && resp.HTTPResponse.Body != nil {
+ // Read error details if possible
+ defer resp.HTTPResponse.Body.Close()
+ body, err := io.ReadAll(resp.HTTPResponse.Body)
+ if err == nil && len(body) > 0 {
+ errorMsg = string(body)
+ }
+ }
+ return "", fmt.Errorf("API returned error status: %d, details: %s", resp.StatusCode(), errorMsg)
}Don't forget to add the import for io.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func upsertPolicy( | |
| ctx context.Context, | |
| client *api.ClientWithResponses, | |
| policy api.UpsertPolicyJSONRequestBody, | |
| ) (string, error) { | |
| resp, err := client.UpsertPolicyWithResponse(ctx, policy) | |
| if err != nil { | |
| return "", fmt.Errorf("API request failed: %w", err) | |
| } | |
| if resp.StatusCode() >= 400 { | |
| return "", fmt.Errorf("API returned error status: %d", resp.StatusCode()) | |
| } | |
| if resp.JSON200 != nil { | |
| return resp.JSON200.Id.String(), nil | |
| } | |
| return "", fmt.Errorf("unexpected response format") | |
| } | |
| import ( | |
| // ... other imports ... | |
| "io" | |
| ) | |
| func upsertPolicy( | |
| ctx context.Context, | |
| client *api.ClientWithResponses, | |
| policy api.UpsertPolicyJSONRequestBody, | |
| ) (string, error) { | |
| resp, err := client.UpsertPolicyWithResponse(ctx, policy) | |
| if err != nil { | |
| return "", fmt.Errorf("API request failed: %w", err) | |
| } | |
| if resp.StatusCode() >= 400 { | |
| errorMsg := "Unknown error" | |
| if resp.HTTPResponse != nil && resp.HTTPResponse.Body != nil { | |
| // Read error details if possible | |
| defer resp.HTTPResponse.Body.Close() | |
| body, err := io.ReadAll(resp.HTTPResponse.Body) | |
| if err == nil && len(body) > 0 { | |
| errorMsg = string(body) | |
| } | |
| } | |
| return "", fmt.Errorf("API returned error status: %d, details: %s", resp.StatusCode(), errorMsg) | |
| } | |
| if resp.JSON200 != nil { | |
| return resp.JSON200.Id.String(), nil | |
| } | |
| return "", fmt.Errorf("unexpected response format") | |
| } |
🤖 Prompt for AI Agents
In cmd/ctrlc/root/apply/policy.go around lines 132 to 152, enhance the error
handling in upsertPolicy by extracting and including detailed error information
from the API response body when the status code indicates an error (>= 400).
Modify the error return to include this detail instead of only the status code.
Also, add the necessary import for the "io" package to support reading the
response body.
| return api.UpsertPolicyJSONRequestBody{ | ||
| Name: policy.Name, | ||
| Description: policy.Description, | ||
| Priority: policy.Priority, | ||
| Enabled: policy.Enabled, | ||
| WorkspaceId: workspace, | ||
| Targets: targets, | ||
| DenyWindows: &denyWindows, | ||
| DeploymentVersionSelector: deploymentVersionSelector, | ||
| VersionAnyApprovals: &versionAnyApprovals, | ||
| VersionUserApprovals: &versionUserApprovals, | ||
| VersionRoleApprovals: &versionRoleApprovals, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper validation of policy data
The code doesn't validate that required fields like Name and Targets are non-empty before sending to the API. While the API might perform validation, early validation in the CLI would provide better user experience.
Add validation before creating the request body:
func processPolicy(
ctx context.Context,
client *api.ClientWithResponses,
policy Policy,
policyWg *sync.WaitGroup,
) {
defer policyWg.Done()
+ // Validate required fields
+ if policy.Name == "" {
+ log.Error("Failed to create policy: name is required", "policy", policy)
+ return
+ }
+
+ if len(policy.Targets) == 0 {
+ log.Error("Failed to create policy: at least one target is required", "name", policy.Name)
+ return
+ }
+
body := createPolicyRequestBody(policy)
// ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return api.UpsertPolicyJSONRequestBody{ | |
| Name: policy.Name, | |
| Description: policy.Description, | |
| Priority: policy.Priority, | |
| Enabled: policy.Enabled, | |
| WorkspaceId: workspace, | |
| Targets: targets, | |
| DenyWindows: &denyWindows, | |
| DeploymentVersionSelector: deploymentVersionSelector, | |
| VersionAnyApprovals: &versionAnyApprovals, | |
| VersionUserApprovals: &versionUserApprovals, | |
| VersionRoleApprovals: &versionRoleApprovals, | |
| } | |
| func processPolicy( | |
| ctx context.Context, | |
| client *api.ClientWithResponses, | |
| policy Policy, | |
| policyWg *sync.WaitGroup, | |
| ) { | |
| defer policyWg.Done() | |
| // Validate required fields | |
| if policy.Name == "" { | |
| log.Error("Failed to create policy: name is required", "policy", policy) | |
| return | |
| } | |
| if len(policy.Targets) == 0 { | |
| log.Error("Failed to create policy: at least one target is required", "name", policy.Name) | |
| return | |
| } | |
| body := createPolicyRequestBody(policy) | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In cmd/ctrlc/root/apply/policy.go around lines 117 to 129, the code constructs
an UpsertPolicyJSONRequestBody without validating required fields such as Name
and Targets. Add checks before this return statement to ensure that policy.Name
is not empty and that targets is not empty or nil. If validation fails, return
an appropriate error or prompt the user, preventing the API call with invalid
data and improving user experience.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cmd/ctrlc/root/apply/policy.go (2)
40-44: Still missing validation of required fields besidesNamePrevious reviews pointed out that
Targets(and potentially other required
fields) should be validated before the API call. The comment remains
unaddressed.
148-150: Return API error details to aid troubleshootingEarlier feedback suggested reading the response body so the user sees the
actual API error instead of only the status code; this has not yet been
implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
cmd/ctrlc/root/apply/cmd.go(1 hunks)cmd/ctrlc/root/apply/policy.go(1 hunks)cmd/ctrlc/root/apply/types.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/ctrlc/root/apply/cmd.go
- cmd/ctrlc/root/apply/types.go
| for _, policy := range policies { | ||
| wg.Add(1) | ||
| policy.WorkspaceId = workspaceID | ||
| go processPolicy(ctx, client, policy, &wg) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid clobbering a workspace ID provided in YAML
policy.WorkspaceId = workspaceID always overwrites the value that may already be present on the incoming Policy object.
That makes it impossible for users to set workspaceId per-policy in their YAML file – behaviour that other resources honour.
Consider preserving the YAML value and only falling back to the CLI/flag value when the field is empty:
- policy.WorkspaceId = workspaceID
+ if policy.WorkspaceId == "" {
+ policy.WorkspaceId = workspaceID // fallback only
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, policy := range policies { | |
| wg.Add(1) | |
| policy.WorkspaceId = workspaceID | |
| go processPolicy(ctx, client, policy, &wg) | |
| for _, policy := range policies { | |
| wg.Add(1) | |
| if policy.WorkspaceId == "" { | |
| policy.WorkspaceId = workspaceID // fallback only | |
| } | |
| go processPolicy(ctx, client, policy, &wg) | |
| } |
🤖 Prompt for AI Agents
In cmd/ctrlc/root/apply/policy.go around lines 24 to 27, the code
unconditionally overwrites the policy's WorkspaceId with the workspaceID
variable, ignoring any value set in the YAML. Modify the code to check if
policy.WorkspaceId is empty before assigning workspaceID, so that the YAML value
is preserved if present and the CLI/flag value is used only as a fallback.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes