Skip to content
This repository was archived by the owner on Mar 17, 2026. It is now read-only.

fix(engines): ensure platform libraries can't be defined multiple times#90

Merged
jyecusch merged 2 commits intomainfrom
fix-lib-aliases
Sep 26, 2025
Merged

fix(engines): ensure platform libraries can't be defined multiple times#90
jyecusch merged 2 commits intomainfrom
fix-lib-aliases

Conversation

@jyecusch
Copy link
Copy Markdown
Member

No description provided.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 26, 2025

Walkthrough

This PR introduces typed identifiers for libraries and plugins in the Terraform engine. It adds libraryID and libraryVersion types, replaces PlatformSpec.Libraries keys/values accordingly, and updates GetLibrary/GetLibraries signatures to use libraryID. A new pluginSource type is added, and ResourceBlueprint now uses Source instead of PluginId, with ResolvePlugin updated to validate and resolve via the new types. Additional nil checks and stricter error handling are added in TerraformEngine and platform code. Error messages across deployment, resource handling, and plugin resolution paths are expanded to include contextual details.

Possibly related PRs

Suggested labels

released

Suggested reviewers

  • davemooreuws
  • tjholm
  • HomelessDinosaur
  • sean-nitric

Pre-merge checks

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The pull request title states that it prevents duplicate platform library definitions, but the changes primarily introduce a new libraryID type, pluginSource handling, and refactor error wrapping without adding explicit duplicate‐definition checks or enforcement logic. As a result, the title misrepresents the core intent and scope of the diff. Please revise the title to accurately reflect the main changes, such as the introduction of the libraryID concept and pluginSource refactor in the Terraform engine, and remove the implication of duplicate‐library enforcement.
Description Check ⚠️ Warning The pull request has no description provided, leaving it impossible to verify relevance or intent with the changeset. An empty description offers no context for the refactoring of libraryID types, pluginSource updates, or error wrapping enhancements. Please add a summary describing the high‐level purpose of this refactor, including key changes in platform.go, infrastructure resource resolution updates, and improved error handling.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

Actionable comments posted: 1

🧹 Nitpick comments (2)
engines/terraform/terraform.go (1)

59-62: Good nil-guard; prevents panic on malformed specs.

Consider mirroring this guard in resolveIdentityPlugin for consistency.

engines/terraform/resource_handler.go (1)

16-19: Runtime nil-check prevents crash. Also validate storage plugin runtimes.

Add a similar guard inside the storage loop to avoid panics when a storage plugin lacks Runtime.

 for name, plug := range storagePlugins {
+  if plug.Runtime == nil {
+    return nil, fmt.Errorf("storage plugin %s has no runtime configuration", name)
+  }
   pluginDef.Storage = append(pluginDef.Storage, plugin.GoPlugin{
     Alias:  fmt.Sprintf("storage_%s", name),
     Name:   name,
     Import: strings.Split(plug.Runtime.GoModule, "@")[0],
   })
   gets = append(gets, plug.Runtime.GoModule)
 }

Optional: dedupe gets to avoid duplicate module fetches (nice-to-have).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 97a5a74 and 8dc70f8.

📒 Files selected for processing (4)
  • engines/terraform/deployment.go (3 hunks)
  • engines/terraform/platform.go (4 hunks)
  • engines/terraform/resource_handler.go (5 hunks)
  • engines/terraform/terraform.go (2 hunks)
🔇 Additional comments (9)
engines/terraform/terraform.go (1)

81-81: Clear error wrapping with context.

Good use of %w and inclusion of type/intent.

engines/terraform/resource_handler.go (5)

62-63: Better error context for service identities.

Message is consistent with the new wrapping pattern.


84-85: Good contextual error for service resource plugin resolution.


122-123: Good contextual error for bucket plugin resolution.


176-177: Good contextual error for entrypoint plugin resolution.


206-208: Good contextual error for database plugin resolution.

engines/terraform/deployment.go (3)

70-71: Helpful error wrapping for infra plugin resolution.

Keeps infra name in the error path.


88-89: Clearer error when route target missing.

Nice inclusion of entrypoint and target names.


98-99: Clearer error for invalid target type.

Consistent with other contextual errors.

Comment thread engines/terraform/platform.go
@jyecusch jyecusch merged commit 0542b92 into main Sep 26, 2025
3 checks passed
@jyecusch jyecusch deleted the fix-lib-aliases branch September 26, 2025 02:55
@nitric-bot
Copy link
Copy Markdown

🎉 This PR is included in version 0.1.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants