Skip to content

chore: surface error if getProviderId called on resource without proivder#67

Open
adityachoudhari26 wants to merge 1 commit intomainfrom
err-if-no-provider
Open

chore: surface error if getProviderId called on resource without proivder#67
adityachoudhari26 wants to merge 1 commit intomainfrom
err-if-no-provider

Conversation

@adityachoudhari26
Copy link
Copy Markdown
Member

@adityachoudhari26 adityachoudhari26 commented Mar 27, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Resource upsert operations now fail with a clear error message when a provider is not declared, instead of silently proceeding with a default value. This improvement helps catch configuration issues earlier and provides better visibility into resource setup.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

The getProviderID method in ResourceItemSpec now raises an error when r.Provider is empty, removing the implicit default fallback to "ctrlc-apply". Resource upsert operations must now have an explicit provider declaration or fail early.

Changes

Cohort / File(s) Summary
Provider Error Handling
internal/api/providers/resource.go
Modified getProviderID to reject missing provider instead of defaulting to "ctrlc-apply", enforcing explicit provider specification.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A provider once hidden, now seen with care,
No silent defaults to haunt the air,
Speak up or fail, the resource must know,
Explicit is clearer, like morning's soft glow! 🌱

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: adding error handling when getProviderId is called on a resource without a provider, which is the core modification shown in the summary.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch err-if-no-provider

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/api/providers/resource.go (1)

130-134: Consider including resource identifier in error message for easier debugging.

When multiple resources are processed, identifying which one lacks a provider can be difficult with a generic error. Including the identifier would help users quickly locate the problematic resource definition.

Suggested improvement
 func (r *ResourceItemSpec) getProviderID(ctx Context) (string, error) {
 	providerName := r.Provider
 	if providerName == "" {
-		return "", fmt.Errorf("resource has no provider")
+		return "", fmt.Errorf("resource %q has no provider", r.Identifier)
 	}
🤖 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, The error
returned from ResourceItemSpec.getProviderID is too generic; update the function
to include the resource's identifier in the error message so callers can
pinpoint which resource is missing a provider. Locate
ResourceItemSpec.getProviderID and change the error path that returns
fmt.Errorf("resource has no provider") to include a unique field from the
receiver (e.g., r.ID, r.Name, or another identifier available on
ResourceItemSpec) so the message becomes something like "resource <identifier>
has no provider" and still returns the same error type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/api/providers/resource.go`:
- Around line 130-134: The error returned from ResourceItemSpec.getProviderID is
too generic; update the function to include the resource's identifier in the
error message so callers can pinpoint which resource is missing a provider.
Locate ResourceItemSpec.getProviderID and change the error path that returns
fmt.Errorf("resource has no provider") to include a unique field from the
receiver (e.g., r.ID, r.Name, or another identifier available on
ResourceItemSpec) so the message becomes something like "resource <identifier>
has no provider" and still returns the same error type.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 38b94872-ce7d-436b-8250-93359c3871bb

📥 Commits

Reviewing files that changed from the base of the PR and between 20021c3 and b406da0.

📒 Files selected for processing (1)
  • internal/api/providers/resource.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant