-
Notifications
You must be signed in to change notification settings - Fork 489
Plugin request state #2244
Plugin request state #2244
Conversation
026cb04
to
0c4fb76
Compare
f123c1d
to
f5cab77
Compare
I think I've sat on this long enough to decide I don't like the name Also, do we want to start to deprecate I also think the state should include the current selected context. |
Good catch. I totally missed including context. I had considered adding clientID to the state but I didn't add it as it is used in other places separately and it avoided removing clientID from protobuf for existing plugins which might be using it separately. But passing the reference to the state is still a valid solution. I'll update it :) |
2a607bb
to
4f48304
Compare
4f48304
to
8af396d
Compare
lets squash some of these commits |
Signed-off-by: vikram yadav <[email protected]>
Signed-off-by: vikram yadav <[email protected]>
8af396d
to
ca09140
Compare
What this PR does / why we need it: This PR passes a shared partial octant state to the request objects passed to the plugins as arguments.
Which issue(s) this PR fixes
Special notes for your reviewer:
Some Design choices:
The partial state is stored and passed via context. I felt conflicted about this choice as it didn't feel right to pass this data through context rather than a function argument. The reason context was picked to avoid updating multiple function arguments and interface implementation (which doesn't provide any meaning for the interfaces) along the multiple hops it takes for the state to go from state managers to plugins (for example: content_manager to a go plugin takes a lot of hops).
This way is a bit obscure but provides meaningful function params and data can be tracked with as much difficulty as a function argument would provide.
clientId and shared plugin state is kept separate as clientId is used in other places and it doesn't update the existing protobufs.
plugins get the state as
OState
property describing partial octant state.