feat(temporal): remove connector from db when failing to install #282
Conversation
WalkthroughThe pull request introduces changes to the error handling and method signatures related to plugin unregistration across multiple files in the internal connectors engine. The primary modification is the removal of error return from the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
==========================================
+ Coverage 72.91% 72.99% +0.07%
==========================================
Files 537 537
Lines 26988 26998 +10
==========================================
+ Hits 19679 19706 +27
+ Misses 6248 6232 -16
+ Partials 1061 1060 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/connectors/engine/plugins/plugin.go (1)
97-104: Consolidate plugin removal steps or log potential anomalies.
The method no longer returns an error, so callers must rely on logs or other observable mechanisms if unregistration fails internally. In scenarios where partial failures could occur (e.g., resources not fully released), consider adding structured logs or metrics to track issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/connectors/engine/plugins/plugin.go(2 hunks)internal/connectors/engine/plugins/plugin_generated.go(1 hunks)internal/connectors/engine/workers.go(1 hunks)internal/connectors/engine/workflow/install_connector.go(1 hunks)internal/connectors/engine/workflow/install_connector_test.go(7 hunks)internal/connectors/engine/workflow/uninstall_connector.go(1 hunks)internal/connectors/plugins/registry/wrapper.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (11)
internal/connectors/engine/plugins/plugin_generated.go (1)
88-90: Confirm internal error handling or logging.
By removing the error return, any potential issue withinUnregisterPluginwon’t be conveyed to the caller. If the implementation needs to signal errors or unexpected conditions, consider logging them within this method.internal/connectors/engine/plugins/plugin.go (1)
30-30: Interface signature updated to remove error return.
The new signatureUnregisterPlugin(models.ConnectorID)aligns with the changed contract. Verify that all consumers of this interface properly handle the absence of any return status or error.
[approve]internal/connectors/engine/workflow/install_connector.go (2)
23-31: Good defensive cleanup on installation failure.
Removing the connector from the database ensures the system remains consistent if installation fails. This prevents orphaned connectors.
33-33: No error handling for plugin unregistration.
CallingUnregisterPluginhere follows the updated contract. However, if any cleanup step is needed internally (e.g., removing references from memory), confirm it is handled within the method itself.internal/connectors/engine/workflow/uninstall_connector.go (1)
202-202: Simplified control flow for plugin unregistration.
The direct call toUnregisterPluginwithout error handling is consistent with the updated method signature. Ensure any failures or anomalies that might arise from unregistration are logged or handled using other means.internal/connectors/engine/workers.go (1)
207-207: Consider verifying plugin unregistration outcome.While
UnregisterPluginno longer returns an error, you may want to log or confirm its success to detect potential silent failures or unexpected states.internal/connectors/engine/workflow/install_connector_test.go (4)
68-70: Enhanced error message and cleanup logic look consistent.Changing the error string from "test" to "test-error" provides more clarity, and adding a delete call for the connector on failure ensures cleanup. This improves debuggability and resilience.
Also applies to: 80-80
95-97: Error specification improvements.Using a more specific error message ("test-error") and ensuring the connector is removed upon failure help maintain consistency across test scenarios.
Also applies to: 107-107
122-124: Refined error messages enhance clarity.Aligning the error checks to "test-error" clarifies the failing scenario. The additional remove call on error is consistent with the overall cleanup strategy.
Also applies to: 134-134
137-163: Good addition of a test case for failing connector deletion.Introducing a test path where
StorageConnectorsDeleteActivityitself fails (“test-error-connector”) significantly strengthens coverage and ensures expected workflow termination behavior.internal/connectors/plugins/registry/wrapper.go (1)
36-36: Confirm logger’s support for “%w” in error messages.In Go’s standard library, “%w” is used to wrap errors with
fmt.Errorf. Some logging frameworks do not fully support unwrapping with “%w”. Verify that this change meets your expectations for error formatting.
No description provided.