Remove last remaining connection to kubeops from dashboard. #3812
Conversation
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
There was a problem hiding this comment.
I have a slight preference for avoiding "breaking" changes in a patch release. Considering a possible scenario where someone is using the kubeops API directly, dropping support for it in the upcoming version is too heavy, IMO.
I'd just conditionally render the nginx rules based on the enabled property of the kubeops deployment (or sth similar). See what you think!
Re: removing it from the UI code, 100% agree
| # Ex: from "/api/assetsvc/what$2Fever?param=value" | ||
| # it matches as $1="/what$2Fever" and $args="param=value" | ||
| # downstream services will receive "/assetsvc/what$2Fever?param=value" | ||
| location ~* /api/assetsvc { |
There was a problem hiding this comment.
Although, in theory, it is safe to remove this piece of code, I wouldn't do it in a soon-to-be patch release. What if we just render this excerpt if assetsvc.enabled=true ? And it is in the next minor that we effectively remove it?
There was a problem hiding this comment.
Yeah true. I'd forgotten that people could potentially be using the current kubeops outside of our dashboard usage. +1 for just putting it behind a flag like you've done for assetsvc.
| proxy_pass {{ include "kubeapps.frontend-config.proxy_pass" . -}}; | ||
| } | ||
|
|
||
| location ~* /api/kubeops { |
There was a problem hiding this comment.
Idem here. I'd conditionally render that part based on a kubeops.enabled thing rather than removing it.
|
|
||
| proxy_pass {{ include "kubeapps.frontend-config.proxy_pass" . -}}; | ||
| } | ||
|
|
||
| location ~* /apis { |
There was a problem hiding this comment.
Well, this is also a reminiscence of our beloved Kubeops service, I guess, but it is mostly aimed at handling can-i and other auth stuff mainly, I think?
There was a problem hiding this comment.
Actually, I think it's the one further down (/api/ not /apis/, the latter being kubeapps-apis)? But yes, you're right - that one has a proxy_pass of:
proxy_pass {{ include "kubeapps.frontend-config.proxy_pass" . -}};
which I missed because I was grepping for kubeops in the config. But this helper does indeed just proxy to the kubeops service (not sure why it's labeled frontend-config).
| @@ -100,16 +98,4 @@ export class App { | |||
| installedPackageRef, | |||
| } as DeleteInstalledPackageRequest); | |||
| } | |||
|
|
|||
| // TODO(agamez): remove it once we return the generated resources as part of the InstalledPackageDetail. | |||
Also conditionally removes kubeops from install. Signed-off-by: Michael Nelson <minelson@vmware.com>
PTAL, I've done this instead (as well as updated the assetsvc frontend config stanza to be conditional on its similar enabled flag). Also put all the kubeops resources within a similar condition (matching assetsvc). Only difference is that kubeops.enabled of course currently defaults to true. Thanks Antonio. |
| ## @param kubeops.enabled Specifies whether this component should be installed. | ||
| ## In a future release we will be setting this to default to false. | ||
| ## | ||
| enabled: true |
There was a problem hiding this comment.
Oops, new param... let me re-run the readme generator to also add it here
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Awesome, thank you for taking it into account. I mean, I really want to get rid of the old kubeops code (and some logic it calls under I've added a commit after executing the readminator . |
|
Are there docs anywhere on migrating from old versions where we relied on kubeops, to newer versions that use kubeappasapis? |
|
The migration at the time was transparent to the user... we'd gradually moved API calls from the Kubeapps dashboard client across to the new apis service during a few releases until this PR where we removed the last API call. Unless you're calling APIs on the kubeops service yourself explicitly from your own client or something, there should be no migration needed? If you are doing so, let us know your usage, happy to point you in the right direction to use the new API? |
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Now that the dashboard is using the helm plugin for all helm-related functionality, we have no need for the connection to the kubeops service.
This PR just removes the two small references which are unused afaict.
With this, I believe we can indeed remove the kubeops (and already assetsvc) services? Not sure if we want to do that as part of this release. We'd have to update the internal content team tests I assume. What do you think Antonio?
I've additionally removed the config from the frontend so that no requests are forwarded to the kubeops or assetsvc services to see if anything breaks.
Benefits
Less moving parts :)
Possible drawbacks
None here, but if we remove the kubeops/assetsvc from the code and chart, we'll need to update the content teams tests or similar.
Applicable issues
Additional information