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

ResourceRef confusion in operator support #6062

Open
absoludity opened this issue Mar 8, 2023 · 2 comments
Open

ResourceRef confusion in operator support #6062

absoludity opened this issue Mar 8, 2023 · 2 comments
Labels
component/ui Issue related to kubeapps UI kind/enhancement An issue that reports an enhancement for an implemented feature

Comments

@absoludity
Copy link
Contributor

absoludity commented Mar 8, 2023

Summary
While working on #6013, particularly replacing the frontend code with the connect grpc-web client, I've hit a lint type issue because with the previously generated files, the ResourceRef object was apparently interchangeable with the shared/ResourceRef object which was still used for operator support, and included extra fields (filter, namespaced etc.) With the new generated files, the code works fine in tests, but tsc won't allow these two objects to be used interchangeably.

I don't want to risk an unintended change within the large PR that I'm doing, so will instead disable the lint for this item specifically in my PR have extended our custom ResourceRef from the client one, so that I can come back and look at removing our custom one in isolation with a focused PR and proper testing.

Description
The main place this arises in in the OperatorInstance.tsx where the resulting resource refs are collected using an IAppViewResourceRefs:

let result: IAppViewResourceRefs = {
ingresses: [],
deployments: [],
statefulsets: [],
daemonsets: [],
otherResources: [],
services: [],
secrets: [],
};

This interface uses the client ResourceRef class, yet the OperatorInstance code adds the shared/ResourceRef annotated objects, which are not compatible due to the missing methods and constructor etc.

Additional Info

CustomAppView appears to use the extra namespaced field in tests (but doesn't appear to need to) Similarly, Kube.test.ts appears to use namespaced unnecessarily. Need to audit whether any of the extra fields are used elsewhere. The test "updates the state with the CRD resources" fails if using normal resourcerefs here, because of the missing filter, but again, need to audit where this filter field of the ResourceRef is used in the actual code outside tests.

@absoludity
Copy link
Contributor Author

absoludity commented Mar 9, 2023

It wasn't actually a lint error but an actual tsc compile error. So no warning or lint error to suppress. Instead I've updated our internal ResourceRef with the extra fields to be a child (extends) of the client ResourceRef so that they can be used as the same type (polymorphism). I still want to come back to remove our extended version, if possible (ie. if it's no longer being used)..

absoludity added a commit that referenced this issue Mar 14, 2023
<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

This PR switches the dashboard over to use the new connect generated
grpc-web client and associated generated classes.

It was pretty tedious, with some complexity due to the different
generated classes and client.

## Non-tedious actual changes
- Updating the way the proto one-of (single choice for different values)
work throughout to match the new library (which actually ensures there
is only a single value, actually using a `OneOf` concept, whereas the
old improbable library just allowed all the options at once). This was
quite confusing to change in the actual code functionality as the code
is written as if all options may be specified, but I didn't want to
change that in this PR, just switch.
- Updating the `shared/ResourceRef` class to be a child of the generated
`ResourceRef` class so that we can continue to use it in place of the
generated one as we're currently doing (compiler won't accept it
otherwise now). Created #6062 to track removing our shared one if we
can.
- Updating the method used in the frontend to stream resources (and
changes) for an installed package, since improbable used subscriptions
while the connect one uses async iterators (need to verify this works,
since I don't think we test it in CI, but it should just fall back to
polling if it doesn't).
- Update the `KubeappsGRPCClient` to use the new client (and the
different way of setting headers/metadata).

## Tedious parts

- Any object that is generated from the proto messages now needs to be
instantiated with, for example, `new AvailablePackageSummary({...})` to
pass the compilation, rather than the previous `{...} as
AvailablePackageSummary` as this type assertion fails due to the missing
methods.
- Updating all the generated class imports with `_pb` to import the new
versions (without deleting the old in this PR, as that blows it out by
16k lines).
- Updating all the enums to the more sensible names used by the new lib
- Update a lot of tests that used jest to spy on the old implementation.
- Update all API method names since the connect one uses snake-case with
an initial lower case :/

### Benefits

<!-- What benefits will be realized by the code change? -->

Let's see what CI says, but possibly removing 16k lines of the old
generated code (I'll do that in a follow-up PR). The main benefit is
that we'll be using a supported grpc-web typescript client and can
remove the old improbable generation in the kubeapps apis service.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

It's a lot of changes, there maybe unexpected side-effects.

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #6013 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

---------

Signed-off-by: Michael Nelson <minelson@vmware.com>
@stale
Copy link

stale bot commented Mar 24, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Automatic label to stale issues due inactivity to be closed if no further action label Mar 24, 2023
@absoludity absoludity added component/ui Issue related to kubeapps UI kind/enhancement An issue that reports an enhancement for an implemented feature labels Mar 26, 2023
@stale stale bot removed the stale Automatic label to stale issues due inactivity to be closed if no further action label Mar 26, 2023
@ppbaena ppbaena added this to the Technical debt milestone Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ui Issue related to kubeapps UI kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
Status: 🗂 Backlog
Development

No branches or pull requests

3 participants
@absoludity @ppbaena and others