-
Notifications
You must be signed in to change notification settings - Fork 66
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
stub: pass context to plugins, pass updated resources to UpdateContainers. #40
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
+ Coverage 64.00% 64.50% +0.50%
==========================================
Files 9 9
Lines 1800 1800
==========================================
+ Hits 1152 1161 +9
+ Misses 497 493 -4
+ Partials 151 146 -5 ☔ View full report in Codecov by Sentry. |
0067be5
to
e4a4790
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.
couple thoughts
e4a4790
to
81f7daf
Compare
7d2910a
to
0ba9e87
Compare
Don't hide context from plugins, pass the corresponding context to each plugin handler. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Update mock plugin to implement the updated stub interfaces. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Update plugin event handlers with the updated stub interfaces. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
0ba9e87
to
d65f23e
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.
nit.. commit header for fix plugin UpdateContainerInterface also states
"Refuse instantiating a Stub for any plugin
that implements the old interface."
Fix UpdateContainer adding an argument for the updated resources. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Add basic tests for UpdateContainer using container creation tests as a starting point. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
d65f23e
to
01d5f14
Compare
Argh... Thanks for spotting that ! Fixed. |
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
***: requires version tag, and container runtime release notes (containerd/crio) for breaking api change when they vendor the next tag |
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
Don't hide the ttRPC request context from plugins. Pass the context on to the plugin's request handler. Once we have opentelemetry ttRPC instrumentation support, this should allow us to properly propagate trace spans and have detailed instrumentation on the plugins side properly nested into the related original request on the runtime side.
While making API-breaking changes, also fix UpdateContainer adding an argument with the updated resources.
Update sample plugins and tests with the updated interfaces and add basic tests for UpdateContainer which we lacked so far.