Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Plugin request state #2244

Merged

Conversation

xtreme-vikram-yadav
Copy link
Contributor

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.

pkg/action/actions.go Show resolved Hide resolved
pkg/action/actions.go Outdated Show resolved Hide resolved
@xtreme-vikram-yadav xtreme-vikram-yadav force-pushed the plugin-request-state branch 2 times, most recently from f123c1d to f5cab77 Compare April 1, 2021 19:10
@wwitzel3
Copy link
Contributor

wwitzel3 commented Apr 6, 2021

I think I've sat on this long enough to decide I don't like the name OState .. it's too vague and isn't immediately obvious what it is. I think ClientState maybe a better choice.

Also, do we want to start to deprecate ClientID .. and move it in to ClientState ? So plugin authors get a ClientState object and it includes ID, Filters, Namespace.

I also think the state should include the current selected context.

@xtreme-vikram-yadav
Copy link
Contributor Author

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 :)

@xtreme-vikram-yadav xtreme-vikram-yadav force-pushed the plugin-request-state branch 2 times, most recently from 2a607bb to 4f48304 Compare April 7, 2021 15:16
@GuessWhoSamFoo
Copy link
Contributor

lets squash some of these commits

vikram yadav added 2 commits April 8, 2021 17:39
Signed-off-by: vikram yadav <yvikram@vmware.com>
Signed-off-by: vikram yadav <yvikram@vmware.com>
@wwitzel3 wwitzel3 merged commit 2461ff3 into vmware-archive:master Apr 9, 2021
@xtreme-vikram-yadav xtreme-vikram-yadav deleted the plugin-request-state branch April 9, 2021 04:13
@xtreme-vikram-yadav xtreme-vikram-yadav mentioned this pull request Apr 21, 2021
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow plugins to get the current label filter and observe changes
3 participants