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: Expect V2 Event DTO from triggers. #616

Merged
merged 2 commits into from
Jan 25, 2021

Conversation

lenny-goodell
Copy link
Member

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?

Triggers and some pipeline function expect a V1 Event model

Issue Number: #595

What is the new behavior?

Triggers and appropriate pipeline functions now expect a V2 Event DTO.

V1 Event model temporarily supported by converting V1 Event to a V2 Event DTO.

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

@lenny-goodell
Copy link
Member Author

Putting this on hold as the DTO that will be received from the MessageBus is changing from EventDto to []AddEventDto

@lenny-goodell lenny-goodell marked this pull request as draft January 7, 2021 17:40
@lenny-goodell lenny-goodell added the hold Intended for PRs we want to flag for ongoing review label Jan 7, 2021
@lenny-goodell lenny-goodell force-pushed the v2-event-dto branch 2 times, most recently from b8d681f to a57b559 Compare January 16, 2021 00:21
@codecov-io
Copy link

codecov-io commented Jan 16, 2021

Codecov Report

Merging #616 (578b4c2) into master (8142930) will increase coverage by 1.26%.
The diff coverage is 80.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   60.98%   62.24%   +1.26%     
==========================================
  Files          31       31              
  Lines        1830     1907      +77     
==========================================
+ Hits         1116     1187      +71     
+ Misses        640      639       -1     
- Partials       74       81       +7     
Impacted Files Coverage Δ
internal/runtime/runtime.go 78.83% <72.22%> (+8.70%) ⬆️
appcontext/context.go 84.44% <93.93%> (+11.36%) ⬆️
pkg/transforms/conversion.go 80.95% <100.00%> (ø)
pkg/transforms/filter.go 92.00% <100.00%> (-0.31%) ⬇️
pkg/transforms/tags.go 100.00% <100.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 8142930...578b4c2. Read the comment docs.

@lenny-goodell lenny-goodell force-pushed the v2-event-dto branch 3 times, most recently from 8c202e8 to 6a64ff4 Compare January 22, 2021 21:02
@lenny-goodell lenny-goodell removed the hold Intended for PRs we want to flag for ongoing review label Jan 22, 2021
@lenny-goodell lenny-goodell marked this pull request as ready for review January 22, 2021 21:03
@lenny-goodell lenny-goodell force-pushed the v2-event-dto branch 2 times, most recently from 6f4c8a6 to ce7c848 Compare January 22, 2021 21:23
Triggers expect a V2 AddEventRequest DTO
Appropriate pipeline functions now expect a V2 Event DTO.

V1 Event model temporarily supported by converting V1 Event to a V2 Event DTO.

closes #595

Signed-off-by: lenny <leonard.goodell@intel.com>
Copy link
Contributor

@jim-wang-intel jim-wang-intel left a comment

Choose a reason for hiding this comment

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

overall looks ok, i saw lots of temporary v1 stuffs and some error checks i think we should put in IMO

appcontext/context_test.go Show resolved Hide resolved
internal/runtime/runtime_test.go Outdated Show resolved Hide resolved
internal/runtime/runtime_test.go Outdated Show resolved Hide resolved
internal/runtime/runtime_test.go Outdated Show resolved Hide resolved
internal/runtime/runtime_test.go Outdated Show resolved Hide resolved
internal/trigger/messagebus/messaging_test.go Outdated Show resolved Hide resolved
internal/trigger/messagebus/messaging_test.go Outdated Show resolved Hide resolved
internal/trigger/messagebus/messaging_test.go Outdated Show resolved Hide resolved
pkg/transforms/filter.go Outdated Show resolved Hide resolved
pkg/transforms/filter_test.go Show resolved Hide resolved
AlexCuse
AlexCuse previously approved these changes Jan 24, 2021
Signed-off-by: lenny <leonard.goodell@intel.com>
Copy link
Contributor

@jim-wang-intel jim-wang-intel 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 2ceec0a into edgexfoundry:master Jan 25, 2021
@lenny-goodell lenny-goodell deleted the v2-event-dto branch January 25, 2021 20:36
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.

Accept V2 Event DTO from triggers (App SDK)
4 participants