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): Enable Custom Trigger Registration #587

Merged

Conversation

AlexCuse
Copy link
Contributor

@AlexCuse AlexCuse commented Dec 6, 2020

accept registration of factory functions for making custom trigger types
available to the SDK

Signed-off-by: Alex Ullrich alexullrich@technotects.com

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)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #586

What is the new behavior?

Allow registering custom trigger builders to be used when initializing trigger to run the service.

Does this PR introduce a breaking change?

  • Yes
  • No

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

No

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

Other information

@AlexCuse AlexCuse changed the title feat(sdk); Enable Custom Trigger Registration feat(sdk): Enable Custom Trigger Registration Dec 6, 2020
@AlexCuse
Copy link
Contributor Author

AlexCuse commented Dec 6, 2020

I used the same string/factory function map approach as in edgexfoundry/go-mod-messaging#75

Happy to do something more robust if need be.

I was torn on whether or not to turn MessageError into a normal error for return from ProcessMessageFunc and decided to keep it simple for now but that is easy enough to add as well.

@codecov-io
Copy link

codecov-io commented Dec 6, 2020

Codecov Report

Merging #587 (9df6351) into master (fb6bb54) will increase coverage by 0.04%.
The diff coverage is 64.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
+ Coverage   61.76%   61.80%   +0.04%     
==========================================
  Files          31       32       +1     
  Lines        1810     1846      +36     
==========================================
+ Hits         1118     1141      +23     
- Misses        619      631      +12     
- Partials       73       74       +1     
Impacted Files Coverage Δ
appsdk/sdk.go 32.86% <ø> (-1.94%) ⬇️
appsdk/triggerfactory.go 64.00% <64.00%> (ø)

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 fb6bb54...9df6351. Read the comment docs.

@AlexCuse
Copy link
Contributor Author

Before I rebase I think I'm going to add an internal method to the SDK to provide a 'starter' appcontext seeded with clients, config etc.....

@AlexCuse
Copy link
Contributor Author

That should be better I just changed the builder function to take the same signature as the MQTT trigger

@AlexCuse AlexCuse force-pushed the custom-trigger-registration branch 3 times, most recently from c7b974f to f81c328 Compare December 12, 2020 00:20
@lenny-goodell lenny-goodell linked an issue Dec 14, 2020 that may be closed by this pull request
@AlexCuse AlexCuse force-pushed the custom-trigger-registration branch 2 times, most recently from 7a3b32b to 123c7b3 Compare December 17, 2020 13:08
@AlexCuse
Copy link
Contributor Author

Sorry for the chatter on this one - I wanted to check if anyone had any ideas as I worked through it. I've settled on a pair of functions to pass from the SDK to custom triggers - one to process messages on the runtime, and another to build an appcontext.Context for a given message. Its still a little awkward - I'm trying to not expose the BindingInfo type I need for the trigger, so I access it on construction by creating a fake appcontext.Context and reading through the configuration pointer available there. Other than that it feels pretty decent to use.

I think the context building function may be useful for #599 as well would just be a matter of incorporating in the built in triggers.

@AlexCuse
Copy link
Contributor Author

AlexCuse commented Dec 17, 2020

Here is an example trigger that I have been using to test with kafka: https://github.com/AlexCuse/edgex-watermill/blob/master/core/watermilltrigger.go

@AlexCuse AlexCuse force-pushed the custom-trigger-registration branch 2 times, most recently from fdab45b to 083785a Compare December 22, 2020 16:23
@AlexCuse
Copy link
Contributor Author

I added a new type TriggerConfig that provides a container to pass the necessary bits to construct a trigger from the SDK. Its still a little broad right now (I wonder about how to get to a generic broker config - I just use MessageBus currently) but better than opening up the whole SDK I think.

@AlexCuse
Copy link
Contributor Author

This branch should be up to date now.

lenny-goodell
lenny-goodell previously approved these changes Jan 7, 2021
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.

Overall this is great! Just a few minor nits. ;-)

appsdk/trigger.go Outdated Show resolved Hide resolved
appsdk/triggerfactory.go Outdated Show resolved Hide resolved
appsdk/triggerfactory_test.go Outdated Show resolved Hide resolved
appsdk/triggerfactory_test.go Outdated Show resolved Hide resolved
appsdk/sdk.go Outdated Show resolved Hide resolved
appsdk/triggerfactory_test.go Outdated Show resolved Hide resolved
appsdk/triggerfactory_test.go Outdated Show resolved Hide resolved
@AlexCuse AlexCuse force-pushed the custom-trigger-registration branch 2 times, most recently from 95f303c to 8b097a4 Compare January 7, 2021 22:38
accept registration of factory functions for making custom trigger types
available to the SDK

Signed-off-by: Alex Ullrich <alexullrich@technotects.com>
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 8220514 into edgexfoundry:master Jan 8, 2021
AlexCuse pushed a commit to AlexCuse/edgex-examples that referenced this pull request Jan 11, 2021
Introduce an example for the custom trigger functionality introduced to
the SDK in edgexfoundry/app-functions-sdk-go#587
AlexCuse pushed a commit to AlexCuse/edgex-examples that referenced this pull request Jan 11, 2021
Introduce an example for the custom trigger functionality introduced to
the SDK in edgexfoundry/app-functions-sdk-go#587

Signed-off-by: Alex Ullrich <alexullrich@technotects.com>
AlexCuse pushed a commit to AlexCuse/edgex-examples that referenced this pull request Jan 11, 2021
Introduce an example for the custom trigger functionality introduced to
the SDK in edgexfoundry/app-functions-sdk-go#587

Signed-off-by: Alex Ullrich <alexullrich@technotects.com>
AlexCuse pushed a commit to AlexCuse/edgex-examples that referenced this pull request Jan 11, 2021
Introduce an example for the custom trigger functionality introduced to
the SDK in edgexfoundry/app-functions-sdk-go#587

Signed-off-by: Alex Ullrich <alexullrich@technotects.com>
AlexCuse pushed a commit to AlexCuse/edgex-examples that referenced this pull request Jan 11, 2021
Introduce an example for the custom trigger functionality introduced to
the SDK in edgexfoundry/app-functions-sdk-go#587
Signed-off-by: Alex Ullrich <alexullrich@technotects.com>
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.

bug(sdk): triggers don't set secret provider on appcontext passed to functions Custom Trigger Support
3 participants