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

Switch Dashboard to use new gRPC client throughout. #6044

Merged
merged 20 commits into from
Mar 14, 2023

Conversation

absoludity
Copy link
Contributor

@absoludity absoludity commented Mar 3, 2023

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 ResourceRef confusion in operator support #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

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

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

Applicable issues

Additional information

@absoludity absoludity marked this pull request as ready for review March 7, 2023 23:40
Base automatically changed from use-buf-grpc-web to main March 8, 2023 02: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>
Lint
Signed-off-by: Michael Nelson <minelson@vmware.com>
@netlify
Copy link

netlify bot commented Mar 8, 2023

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit a327ed2
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/64100edf5946a40008d5743a

Signed-off-by: Michael Nelson <minelson@vmware.com>
type error.

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>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity
Copy link
Contributor Author

absoludity commented Mar 10, 2023

I've got the image compiling now, but haven't yet sorted out the slightly different behavior for our login checks, which is the cause of the e2e failures.

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

absoludity commented Mar 13, 2023

I've got the image compiling now, but haven't yet sorted out the slightly different behavior for our login checks, which is the cause of the e2e failures.

Turned out to be a simple difference in the way headers are extracted from a grpc-web response. Locally I checked everything works, including the bits that had changed (long-running requests), let's see what CI thinks.

Signed-off-by: Michael Nelson <minelson@vmware.com>
according to the new format.

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

I've got the image compiling now, but haven't yet sorted out the slightly different behavior for our login checks, which is the cause of the e2e failures.

Turned out to be a simple difference in the way headers are extracted from a grpc-web response. Locally I checked everything works, including the bits that had changed (long-running requests), let's see what CI thinks.

This is where I miss team-mates to celebrate with :P. After a lot of debugging, found what is hopefully the final CI issue - simply that the incoming error was not being converted correctly because the new grpc error does not have a headerMap like the old. Should have grepped for headerMap much earlier after changing the code in some places.

@absoludity absoludity merged commit eb82234 into main Mar 14, 2023
@absoludity absoludity deleted the 6031-use-buf-grpc-web-2 branch March 14, 2023 19:39
absoludity added a commit that referenced this pull request 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

<!-- Describe the scope of your change - i.e. what the change does. -->
Just a follow-up to #6044 which removes the now unused code.

### Benefits

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

### Possible drawbacks

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

### 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants