-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(inbound): invalid inbound connector definitions are not stored #2504
fix(inbound): invalid inbound connector definitions are not stored #2504
Conversation
963ed9b
to
be760af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chillleader Thanks for turning this around good work so quickly!
I left a few comments - feel free to answer the non-blocking ones post-merge (or create a new issue for them if you'd like).
I'd appreciate another teammate (@sbuettner or @johnBgood perhaps?) giving the final approval, as I haven't worked with inbound connector code that much yet!
...me-core/src/main/java/io/camunda/connector/runtime/core/inbound/InboundConnectorDetails.java
Outdated
Show resolved
Hide resolved
...me-core/src/main/java/io/camunda/connector/runtime/core/inbound/InboundConnectorDetails.java
Outdated
Show resolved
Hide resolved
...ore/src/test/java/io/camunda/connector/runtime/core/inbound/InboundConnectorDetailsTest.java
Show resolved
Hide resolved
...me-core/src/main/java/io/camunda/connector/runtime/core/inbound/InboundConnectorDetails.java
Outdated
Show resolved
Hide resolved
...test/java/io/camunda/connector/runtime/inbound/executable/InboundExecutableRegistryTest.java
Outdated
Show resolved
Hide resolved
...main/java/io/camunda/connector/runtime/inbound/executable/InboundExecutableRegistryImpl.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/camunda/connector/runtime/inbound/executable/BatchExecutableProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
One last little comment, then feel free to merge!
...mplate-generator/core/src/main/java/io/camunda/connector/generator/dsl/CommonProperties.java
Show resolved
Hide resolved
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release/8.6
git worktree add -d .worktree/backport-2504-to-release/8.6 origin/release/8.6
cd .worktree/backport-2504-to-release/8.6
git switch --create backport-2504-to-release/8.6
git cherry-pick -x 2866c41afbac4846315400a616e10bcd4a96941a |
…2504) * fix(inbound): invalid definitions are not stored for user convenience * fix polling connector test * address review suggestions fix test
…2504) * fix(inbound): invalid definitions are not stored for user convenience * fix polling connector test * address review suggestions fix test
Description
This PR introduces an additional mechanism for capturing errors related to invalid connector configuraitons that happen before creating inbound connector executables (for example, when multiple connectors that have different properties are combined using the same deduplication ID).
Instead of discarding such errors, they are now captured in an
InvalidInboundConnectorDetails
object. TheInboundConnectorDetails
class became a sealed interface. The new sealed hierarchy is needed because invalid inbound connector details won't have the same fields as the valid ones (e.g.properties
will be missing, as they might be different for connector elements in an invalid group).Additionally, this PR changes the validation error message in the element templates that support deduplication. The reason behind this change is that the properties panel shows an error in the following format:
<property label> <error message>
, so the new message fits more naturally.Related issues
closes https://github.com/camunda/team-connectors/issues/762