fix: call regular resource upsert endpoint when no provider is specified#66
Conversation
When `ctrlc apply` is run without a `--provider` flag and a resource YAML
does not have a `provider` field set, the CLI now calls
`PATCH /v1/workspaces/{workspaceId}/resources/identifier/{identifier}`
directly instead of creating/reusing a default "ctrlc-apply" provider.
Changes:
- `--provider` flag default changed from "ctrlc-apply" to "" so that
the absence of a provider is detectable
- Resources without a provider skip the SetResourceProviderResources path
and are upserted individually via the new UpsertResourceByIdentifier helper
- Provider is only assigned from the CLI flag when the flag is explicitly set
Fixes #64
Co-authored-by: Aditya Choudhari <adityachoudhari26@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR removes the default "ctrlc-apply" provider fallback when no Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/client.gen.go (1)
7244-7284:⚠️ Potential issue | 🔴 CriticalUse
PATCHforUpsertResourceByIdentifier.Line 7276 still builds a
PUTrequest, but issue#64requiresPATCH /v1/workspaces/{workspaceId}/resources/identifier/{identifier}for the no-provider path. That means everyUpsertResourceByIdentifier*wrapper will hit the wrong route/semantics, which can 405 or turn the intended additive patch into replacement behavior. Since this file is generated, please fix the OpenAPI operation and regenerate the client.Expected generated output after fixing the spec
- req, err := http.NewRequest("PUT", queryURL.String(), body) + req, err := http.NewRequest("PATCH", queryURL.String(), body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.gen.go` around lines 7244 - 7284, The generated client is creating a PUT request in NewUpsertResourceByIdentifierRequestWithBody (http.NewRequest("PUT", ...)) but the OpenAPI spec requires PATCH for the upsert-by-identifier no-provider path; update the OpenAPI operation for the /v1/workspaces/{workspaceId}/resources/identifier/{identifier} endpoint to use the PATCH HTTP method (or directly change the operationId/method mapping) and then regenerate the client so NewUpsertResourceByIdentifierRequestWithBody (and all UpsertResourceByIdentifier* wrappers) use "PATCH" instead of "PUT".
🧹 Nitpick comments (1)
internal/api/providers/resource.go (1)
130-134: Remove the "ctrlc-apply" fallback ingetProviderID()for consistency.The method still falls back to
"ctrlc-apply"whenProvideris empty. However,BatchUpsertResources()(lines 163–178) explicitly separates empty-provider resources and routes them toupsertWithoutProvider(), which bypasses providers entirely. This creates two inconsistent code paths for the same input:
- Single-resource path (Create/Update via framework.go): Empty provider → fallback to "ctrlc-apply"
- Batch path (BatchUpsertResources): Empty provider → upsertWithoutProvider (no provider)
Either remove the fallback and require a provider, or add a comment explaining that this legacy behavior is not used by the batch flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/providers/resource.go` around lines 130 - 134, Remove the hardcoded "ctrlc-apply" fallback in getProviderID so empty r.Provider is treated as missing; update getProviderID (in ResourceItemSpec) to return an error when Provider is empty instead of returning "ctrlc-apply", and ensure the callers (including any code paths used by Create/Update) surface that error; this makes behavior consistent with BatchUpsertResources which routes empty-provider resources to upsertWithoutProvider (see BatchUpsertResources and upsertWithoutProvider for the intended path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/api/client.gen.go`:
- Around line 7244-7284: The generated client is creating a PUT request in
NewUpsertResourceByIdentifierRequestWithBody (http.NewRequest("PUT", ...)) but
the OpenAPI spec requires PATCH for the upsert-by-identifier no-provider path;
update the OpenAPI operation for the
/v1/workspaces/{workspaceId}/resources/identifier/{identifier} endpoint to use
the PATCH HTTP method (or directly change the operationId/method mapping) and
then regenerate the client so NewUpsertResourceByIdentifierRequestWithBody (and
all UpsertResourceByIdentifier* wrappers) use "PATCH" instead of "PUT".
---
Nitpick comments:
In `@internal/api/providers/resource.go`:
- Around line 130-134: Remove the hardcoded "ctrlc-apply" fallback in
getProviderID so empty r.Provider is treated as missing; update getProviderID
(in ResourceItemSpec) to return an error when Provider is empty instead of
returning "ctrlc-apply", and ensure the callers (including any code paths used
by Create/Update) surface that error; this makes behavior consistent with
BatchUpsertResources which routes empty-provider resources to
upsertWithoutProvider (see BatchUpsertResources and upsertWithoutProvider for
the intended path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98e3d525-0318-4524-ba88-d3da7d6e5a92
📒 Files selected for processing (3)
cmd/ctrlc/root/apply/cmd.gointernal/api/client.gen.gointernal/api/providers/resource.go
1 participant
When
ctrlc applyis run without a--providerflag and a resource YAML does not have aproviderfield, the CLI now callsPATCH /v1/workspaces/{workspaceId}/resources/identifier/{identifier}directly instead of creating/reusing a default "ctrlc-apply" provider.Fixes #64
Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor