Switch AppView to use new getResources. #3872
Conversation
|
Hmm, CI disagrees with my local experience - with app details displayed without the resources. I'll check in the morning. EDIT: Fixed in 79bd8a4 |
absoludity
force-pushed
the
3779-dashboard-1-actions
branch
from
9aaa108
to
0f31c72
Compare
absoludity
force-pushed
the
3779-dashboard-2-appview
branch
from
25b72a1
to
4928ea9
Compare
| @@ -1,4 +1,5 @@ | |||
| import actions from "actions"; | |||
| import { getType } from "typesafe-actions"; | |||
There was a problem hiding this comment.
I've refactored a little in this file because the existing tests aren't testing that actions are dispatched by the view, but rather that the actions themselves are called... this meant that my test passed even though I'd missed dispatching the action. So I've left the existing tests as they are but updated the ones for the functionality that I'm changing so that they actually assert that the correct action is dispatched. The other issue I had from what I'd included in the diff yesterday was that I hadn't enabled the resources plugin :)
There was a problem hiding this comment.
Thanks for the info and refactoring the test. I've just hit the same issue... the error message wasn't clear at all so, almost running out of ideas I decided to check if the resources plugin was enabled... 🤦
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
force-pushed
the
3779-dashboard-2-appview
branch
from
79bd8a4
to
ab8ef43
Compare
|
This PR is the next to merge into master, when reviewed - so if someone has time (I've added you antonio, but no problem if it waits for @castelblanque on Friday or next week... in fact I'll add you both ;) ) |
chart/kubeapps/Chart.yaml
Outdated
| @@ -33,4 +33,4 @@ maintainers: | |||
| name: kubeapps | |||
| sources: | |||
| - https://github.com/kubeapps/kubeapps | |||
| version: 7.6.2-dev0 | |||
| version: 7.6.1-dev1 | |||
There was a problem hiding this comment.
Not sure if decreasing the version is intentional?
There was a problem hiding this comment.
Nope, must have missed the patch when resolving the conflict. Updated.
| @@ -1605,6 +1605,7 @@ kubeappsapis: | |||
| ## | |||
| enabledPlugins: | |||
| - helm | |||
| - resources | |||
| // TODO(minelson): Update to watch only those resources that | ||
| // we're interested in watching change (deployments, statefulsets etc.) | ||
| // and get the rest. | ||
| dispatch(actions.kube.getResources(app!.installedPackageRef!, app!.apiResourceRefs, true)); |
There was a problem hiding this comment.
I guess typescript transpiler is not smart enough to know app is != undefined at this point
There was a problem hiding this comment.
Not sure of the sequence of events, but it appears to be happy without them now.
| // TODO(minelson): Update to watch only those resources that | ||
| // we're interested in watching change (deployments, statefulsets etc.) | ||
| // and get the rest. | ||
| dispatch(actions.kube.getResources(app!.installedPackageRef!, app!.apiResourceRefs, true)); | ||
| return function cleanup() { |
There was a problem hiding this comment.
Nice! btw, we are not usually using cleanup functions. Perhaps at some point, we should revisit it to ensure we don't have any resource left (I recall to have seen some unmount warnings at the console)
There was a problem hiding this comment.
Yeah, it's required here, because unmounting this component triggers this cleanup letting us complete the request (and remove the subscription from the state, which reminds me, I want to remove it from the state as it's no longer necessary to track there. I'll do that in my current branch).
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Switches the AppView to use the new
getResourcesaction (using the resources plugin), with minimal changes to other code.Checking the application detail for the Kubeapps app now only shows a single request for api server's
getResourcesrather than 16 or so watch requests directly to the k8s api server:And creating a new installation watches for updates so that the UI gets updated when pods are ready:
Benefits
Possible drawbacks
See what CI says.
Applicable issues
Additional information
There will be a few PRs to follow removing lots of code, extracting the current
ResourceRefmodel and all its dependencies, removing the request for all resource kinds from the server, and other indicated improvements like not watching all resources etc.BTW: This is the third PR in a series, I've not yet landed the first until I hear that we're good to do so (ie., when the current release tag is final). Let me know :)