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

Add actions for getting resources from resources plugin. #3862

Merged
merged 7 commits into from
Dec 1, 2021

Conversation

absoludity
Copy link
Contributor

Description of the change

This PR adds the new required actions (and handling of those actions in the reducer) for interacting with the new resources plugin to get and watch resources.

This allows a single request to the resources plugin to watch many resources. The streaming rpc call uses server-side events rather than web sockets, with the grpc-web client using rxjs observables which makes the implementation quite tidy.

Benefits

This PR doesn't change the AppView yet, so the only benefit is preparation for the next PR where the AppView will be updated to use the new action.

Possible drawbacks

Applicable issues

Additional information

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Thank you!! This is way simpler than the manual webhook+fallback polling mechanism! Really happy to move the complexity to the existing protocols and backend instead of a heavy frontend!

@@ -44,6 +49,27 @@ export const closeWatchResource = createAction("CLOSE_WATCH_RESOURCE", resolve =
return (ref: ResourceRef) => resolve(ref);
});

// requestResources takes a ResourceRef[] and subscribes to an observable to
// process the responses as they arrive.
export const requestResources = createAction("REQUEST_RESOURCES", resolve => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, it is in this request event that we also perform the actual subscription. Other actions (which are not meant to return any msg-subscription thing) split the request and receive actions, but here, given that the concept of requesting resources implies requesting a subscription, maybe makes sense to leave it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - it is slightly different to the non-subscription pattern such as the existing getResource, out of necessity, but more similar (and based on) the existing getAndWatchResource function (though it's also slightly different as it is getting/watching a single resource only.

The existing non-subscription/non-websocket functions tend to do:

  • dispatch request action that just does book-keeping in the state,
  • do the actual http request
  • dispatch receive action with the result, updating the state.

I think difference that you are pointing out in this new getResources function, the function itself is not doing the actual grpc request, but relying on the reducer to do more than just book-keeping, as the reducer does the grpc-request to get the subscription and then does the book-keeping with this.

Note that the existing getAndWatchResource similarly does the const socket = ref.watchResource() in the reducer, as part of handling the initial dispatched action (called openWatchResource there). What the existing getAndWatchResource and the new getResources share in common is that there is no book keeping state to handle for the request action until the actual request is performed (as the book keeping is on the socket or subscription respectively).

I did think about reducing the difference by doing the actual request and subscription creation:

const observable = Kube.getResources(pkg, refs, watch);
const subscription = observable.subscribe({...});

in the action, and update the dispatched requestResources action to instead include the subscription, but this means that there is no requestResources dispatched until after the actual request has been performed, which means it may it may be skipped entirely (in the case of an error during the request). The only way I think we could get the same pattern is to dispatch a requestResources action that does nothing with the state, then do the request getting the subscription in the action and dispatch a subscribeResources or similar action to do the book keeping with the resulting subscription?

Let me know what you think.

Comment on lines +211 to +218
r.resourceRef!.apiVersion,
r.resourceRef!.kind,
r.resourceRef!.namespace,
r.resourceRef!.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we 100% sure that r does have a resourceRef ? If not, we can just dispatch an error instead of forcing the null check? No strong opinion, though, happy as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll try returning an error so it's handled by the error handler (which dispatches the receiveResourcesError).

@@ -21,6 +21,14 @@ export function fromCRD(
return ref;
}

// TODO: (minelson) Update to use API resourceRef type once old model removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'd use the same comment format to ease the search term when we want to find for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, updated.

@@ -1,5 +1,9 @@
import { JSONSchemaType } from "ajv";
import { RouterState } from "connected-react-router";
// TODO(minelson): Why is lint complaining:
// error Unable to resolve path to module 'rxjs' import/no-unresolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess is something missing in the eslintrc.json (perhaps in import/resolver ?) not sure, I'd have to check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So finally found the right incantation here, from https://stackoverflow.com/a/55280867 . It looks like it was because rxjs is itself written in typescript directly, but node was (?) only looking for js files in node_modules.

Base automatically changed from 3779-required-apiserver-updates to master December 1, 2021 21:08
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity merged commit f314195 into master Dec 1, 2021
@absoludity absoludity deleted the 3779-dashboard-1-actions branch December 1, 2021 22:43
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.

None yet

2 participants