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

Remove custom tshd types and createTshdClient boilerplate #39828

Merged
merged 8 commits into from
Mar 29, 2024

Conversation

gzdunek
Copy link
Contributor

@gzdunek gzdunek commented Mar 26, 2024

e counterpart https://github.com/gravitational/teleport.e/pull/3788

Since we merged #39229, the createTshdClient layer is actually no longer needed, as it only converts the requests and responses between tshd types and generated proto types.
This PR removes this layer, along with the custom tshd types. This makes adding new gRPC methods much simpler: you add it to the .proto file, run make grpc and that's it - you can start using it in the code.

There is unfortunately one downside of removing the custom types: I had to also remove the typed uris. Previously we were able to cast the proto types to our types with as in createTshdClient (which OTOH wasn't too safe). Now there is no createTshdClient, so there is no place to cast the type (and doing it in every call site would be super inconvenient).
I tried to use TypeScript's declaration merging to overwrite the generated *Uri: string types with our typed uris but with no luck - it is not possible to change the type of an existing property - you can only add new ones.

Because of that I had to change types of URIs to strings in uri.ts.

As always, I recommend reviewing commit-by-commit (the last one is unfortunately quite big).

@gzdunek gzdunek added teleport-connect Issues related to Teleport Connect. no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Mar 26, 2024
@gzdunek gzdunek requested review from ravicious and avatus March 26, 2024 12:38
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I reviewed the PR up until web/packages/teleterm/src/ui/DocumentGatewayApp/DocumentGatewayApp.story.tsx, I'll continue the review tomorrow.

It's looking good so far!

@ravicious ravicious self-requested a review March 26, 2024 14:36
Copy link
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

It looks good to me. Mostly just renames and parameter refactors

Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Got to web/packages/teleterm/src/ui/services/fileTransferClient/fileTransferService.ts, will continue tomorrow.

web/packages/teleterm/src/services/tshd/types.ts Outdated Show resolved Hide resolved
@ravicious ravicious self-requested a review March 27, 2024 14:33
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I clicked around the app a bit and everything appears to be fine. 👍

@@ -147,14 +171,51 @@ export class ResourcesService {
return Promise.allSettled(promises);
}

listUnifiedResources(
async listUnifiedResources(
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a return type for this method? It looks like if we add as const to kind, we don't have to do any casting.

Patch
diff --git a/e b/e
index 62fb227d1e..8084bfca7e 160000
--- a/e
+++ b/e
@@ -1 +1 @@
-Subproject commit 62fb227d1ec4b03e226c5ce9cdf63f588e9156b6
+Subproject commit 8084bfca7ed439777af0e9b43946b2e67704a920
diff --git a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts
index b7de3ba213..60b3fbb3cc 100644
--- a/web/packages/teleterm/src/ui/services/resources/resourcesService.ts
+++ b/web/packages/teleterm/src/ui/services/resources/resourcesService.ts
@@ -174,7 +174,7 @@ export class ResourcesService {
   async listUnifiedResources(
     params: types.ListUnifiedResourcesRequest,
     abortSignal: AbortSignal
-  ) {
+  ): Promise<{ nextKey: string; resources: UnifiedResourceResponse[] }> {
     const { response } = await this.tshClient.listUnifiedResources(params, {
       abort: cloneAbortSignal(abortSignal),
     });
@@ -184,28 +184,28 @@ export class ResourcesService {
         .map(p => {
           if (resourceOneOfIsServer(p.resource)) {
             return {
-              kind: 'server',
+              kind: 'server' as const,
               resource: p.resource.server,
             };
           }
 
           if (resourceOneOfIsDatabase(p.resource)) {
             return {
-              kind: 'database',
+              kind: 'database' as const,
               resource: p.resource.database,
             };
           }
 
           if (resourceOneOfIsApp(p.resource)) {
             return {
-              kind: 'app',
+              kind: 'app' as const,
               resource: p.resource.app,
             };
           }
 
           if (resourceOneOfIsKube(p.resource)) {
             return {
-              kind: 'kube',
+              kind: 'kube' as const,
               resource: p.resource.kube,
             };
           }
@@ -214,7 +214,7 @@ export class ResourcesService {
             `Ignoring unsupported resource ${JSON.stringify(p)}.`
           );
         })
-        .filter(Boolean) as UnifiedResourceResponse[],
+        .filter(Boolean),
     };
   }
 }

Base automatically changed from gzdunek/move-proto-comments to master March 29, 2024 09:04
@gzdunek gzdunek force-pushed the gzdunek/remove-custom-tshd-types branch from 56bf00a to 83fe593 Compare March 29, 2024 09:09
@gzdunek gzdunek added this pull request to the merge queue Mar 29, 2024
Merged via the queue into master with commit e60e071 Mar 29, 2024
34 checks passed
@gzdunek gzdunek deleted the gzdunek/remove-custom-tshd-types branch March 29, 2024 09:33
@public-teleport-github-review-bot

@gzdunek See the table below for backport results.

Branch Result
branch/v15 Failed

@public-teleport-github-review-bot

@gzdunek See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry size/sm teleport-connect Issues related to Teleport Connect. ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants