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
events: move types into service package #1072
events: move types into service package #1072
Conversation
When using events, it was found to be fairly unwieldy with a number of extra packages. For the most part, when interacting with the events service, we want types of the same version of the service. This has been accomplished by moving all events types into the events package. In addition, several fixes to the way events are marshaled have been included. Specifically, we defer to the protobuf type registration system to assemble events and type urls, with a little bit sheen on top of add a containerd.io oriented namespace. This has resulted in much cleaner event consumption and has removed the reliance on error prone type urls, in favor of concrete types. Signed-off-by: Stephen J Day <stephen.day@docker.com>
cc @ehazlett |
Codecov Report
@@ Coverage Diff @@
## master #1072 +/- ##
=======================================
Coverage 59.28% 59.28%
=======================================
Files 5 5
Lines 781 781
=======================================
Hits 463 463
Misses 204 204
Partials 114 114 Continue to review full report at Codecov.
|
} | ||
|
||
url := typesPrefix + proto.MessageName(pb) |
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.
this is much better! nice!
e := &event.RuntimeEvent{} | ||
if err := proto.Unmarshal(evt.Event.Value, e); err != nil { | ||
|
||
switch { |
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.
nit: couldn't we check if events.Is(evt.Event, &eventsapi.RuntimeEvent{})
to remove the need for the evloop
label?
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.
even with the switch
, continue
without a label will affect the innermost loop: https://golang.org/ref/spec#Continue_statements
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.
But I agree that a switch is not needed here since there's only one case.
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.
PRs welcome. ;)
LGTM |
ping @dmcgowan |
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.
LGTM
Use clean path for map and comparison.
When using events, it was found to be fairly unwieldy with a number of
extra packages. For the most part, when interacting with the events
service, we want types of the same version of the service. This has been
accomplished by moving all events types into the events package.
In addition, several fixes to the way events are marshaled have been
included. Specifically, we defer to the protobuf type registration
system to assemble events and type urls, with a little bit sheen on top
of add a containerd.io oriented namespace.
This has resulted in much cleaner event consumption and has removed the
reliance on error prone type urls, in favor of concrete types.
Signed-off-by: Stephen J Day stephen.day@docker.com