Skip to content
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

feat(sdk): Custom Trigger Multi-Pipeline Support #941

Conversation

AlexCuse
Copy link
Contributor

@AlexCuse AlexCuse commented Sep 9, 2021

Add service.processMessageOnRuntime to use default pipeline if configured or attempt to use topic matching logic. Deprecate TriggerMessageProcessor and TriggerContextBuilder and inline the existing functions on TriggerConfig as the approach used there will not work with multiple pipelines. Replace with MessageProcessor that only takes the message envelope and builds context(s) as needed.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Related Docs PR now required (if applicable)

Related Docs PR:

If n/a for Docs PR, state why it is not applicable:

What is the current behavior?

Issue Number:
#940

What is the new behavior?

Custom triggers will be able to use the default topic -> pipeline(s) matching logic.

Does this PR introduce a breaking change?

  • Yes
  • No

Only affects custom triggers, the message processor function no longer takes the app context as a parameter. The trigger will no longer be responsible for establishing context just preparing the message envelope.

Are there any new imports or modules? If so, what are they used for and why?

  • Yes
  • No

go-multierror from hashicorp is used to group pipeline errors for return to the trigger.

Are there any specific instructions or things that should be known prior to reviewing?

Other information

@AlexCuse AlexCuse linked an issue Sep 9, 2021 that may be closed by this pull request
@AlexCuse AlexCuse force-pushed the custom-trigger-support-multiple-pipelines branch 2 times, most recently from c81260c to 38ed098 Compare September 9, 2021 01:13
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #941 (b21662f) into main (262cc6a) will decrease coverage by 0.61%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
- Coverage   66.16%   65.54%   -0.62%     
==========================================
  Files          33       33              
  Lines        2503     2525      +22     
==========================================
- Hits         1656     1655       -1     
- Misses        748      771      +23     
  Partials       99       99              
Impacted Files Coverage Δ
internal/app/triggerfactory.go 50.68% <0.00%> (-23.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 262cc6a...b21662f. Read the comment docs.

@AlexCuse AlexCuse force-pushed the custom-trigger-support-multiple-pipelines branch from 38ed098 to b21662f Compare September 9, 2021 01:35
pkg/interfaces/trigger.go Outdated Show resolved Hide resolved
@AlexCuse AlexCuse force-pushed the custom-trigger-support-multiple-pipelines branch 2 times, most recently from 29bcd0b to 1647a4e Compare September 10, 2021 17:15
internal/app/triggerfactory.go Show resolved Hide resolved
pkg/interfaces/trigger.go Outdated Show resolved Hide resolved
@AlexCuse AlexCuse force-pushed the custom-trigger-support-multiple-pipelines branch 2 times, most recently from dafe690 to 034717c Compare September 10, 2021 18:10
Add service.processMessageOnRuntime to use default pipeline if configured or attempt to use topic matching logic.  Deprecate TriggerMessageProcessor and
TriggerContextBuilder and inline the existing functions on TriggerConfig as the approach used there will not work with multiple pipelines. Replace with
MessageProcessor that only takes the message envelope and builds context(s) as needed.

Signed-off-by: Alex Ullrich <[email protected]>
@AlexCuse AlexCuse force-pushed the custom-trigger-support-multiple-pipelines branch from 034717c to 4d23791 Compare September 10, 2021 18:14
@AlexCuse AlexCuse marked this pull request as ready for review September 10, 2021 19:32
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@lenny-goodell lenny-goodell merged commit 18ff6e1 into edgexfoundry:main Sep 10, 2021
FelixTing pushed a commit to FelixTing/app-functions-sdk-go that referenced this pull request Mar 3, 2022
Add service.processMessageOnRuntime to use default pipeline if configured or attempt to use topic matching logic.  Deprecate TriggerMessageProcessor and
TriggerContextBuilder and inline the existing functions on TriggerConfig as the approach used there will not work with multiple pipelines. Replace with
MessageProcessor that only takes the message envelope and builds context(s) as needed.

Signed-off-by: Alex Ullrich <[email protected]>
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.

[App SDK] Custom Trigger Support for Multiple Pipelines
3 participants