Skip to content

Connect: Create dedicated functions for connecting to resources#24445

Merged
gzdunek merged 5 commits intomasterfrom
gzdunek/create-ssh-document-without-command-launcher
Apr 14, 2023
Merged

Connect: Create dedicated functions for connecting to resources#24445
gzdunek merged 5 commits intomasterfrom
gzdunek/create-ssh-document-without-command-launcher

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Apr 12, 2023

This PR makes connecting to the node faster (without using commandLauncher), since we know all the details needed to establish an SSH connection.
I also created functions for dbs and kubes, so we can share the same code between resource tables/search bar.

I'm only unsure about the naming - I wanted to avoid names like createDatabaseDocument as we already have such functions in DocumentsService. They only create documents objects, but here we do more (we change the workspace, create a document and set it as active). It seems to me that all these operations can live in a function connectToX.


For now I left ssh-ssh command as it is used by QuickInput.

@gzdunek gzdunek requested a review from ravicious April 12, 2023 15:11
@gzdunek gzdunek force-pushed the gzdunek/create-ssh-document-without-command-launcher branch from 00c26be to d424957 Compare April 12, 2023 15:16
Copy link
Copy Markdown
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.

Looks good, the only real change I'd make is not using the spread operator.

Comment on lines +35 to +39
connectToDatabase(
appContext,
{ ...db, dbUser },
{ origin: 'resource_table' }
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

db includes a whole bunch of stuff that's not required by the second argument to connectToDatabase. Spreading it like this also makes it harder to find references to fields returned from makeDatabase.

I think it might be a good idea to do something like this:

Suggested change
connectToDatabase(
appContext,
{ ...db, dbUser },
{ origin: 'resource_table' }
);
const { uri, name, protocol } = db;
connectToDatabase(
appContext,
{ uri, name, protocol, dbUser },
{ origin: 'resource_table' }
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I started with a direct assignment, but then switched to the spread operator 😏 Changed.

protocol: string;
dbUser: string;
},
params: {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

idk if it's just me but I'm used to params being "the" params for the given function. I like the current split into an object describing the target and a separate object with irrelevant stuff,

Maybe something like options/opts would work better here? ChatGPT suggests just calling it telemetryOptions/telemetryData, I suppose we could go with just telemetry as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

telemetry sounds good.

const connectionToReuse = ctx.connectionTracker.findConnectionByDocument(doc);

if (connectionToReuse) {
await ctx.connectionTracker.activateItem(connectionToReuse.id, {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I was working on the prototype, I totally forgot that activateItem already changes the workspace as well. 👍

const connectionToReuse = ctx.connectionTracker.findConnectionByDocument(doc);

if (connectionToReuse) {
await ctx.connectionTracker.activateItem(connectionToReuse.id, {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I kinda wish this whole logic was more centralized, as in there would be a single place encapsulating all this logic. Because right now we create a gateway doc here, if a matching connection exists then we pass the execution to connectionTracker which by itself also handles creating a doc if one is missing.

But this is good enough for now and refactoring this would require a much bigger effort.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree :(

server.uri
connectToNode(
appContext,
{ ...server, login },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same consideration as for db above.


import { DocumentOrigin } from './types';

export async function connectToNode(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why node and not server? I know that we call them nodes on the Teleport side but AFAIK through Connect codebase we consistently call them servers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, changed to server.

perform: login =>
connectToNode(
ctx,
{ ...result.resource, login },
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I mentioned, it'd probably be a good idea to be more explicit here and other places in this file as well with regards to not spreading and instead explicitly listing needed fields.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm only unsure about the naming - I wanted to avoid names like createDatabaseDocument as we already have such functions in DocumentsService. They only create documents objects, but here we do more (we change the workspace, create a document and set it as active). It seems to me that all these operations can live in a function connectToX.

I think that's a good decision. I was wondering what would be the best place for those "global" functions. Previously, the command launcher was a place for those global actions, at the moment I think WorkspacesService is good enough to hold that.

@gzdunek gzdunek marked this pull request as ready for review April 14, 2023 09:28
@gzdunek gzdunek requested a review from avatus April 14, 2023 09:28
@github-actions github-actions Bot requested review from rudream and ryanclark April 14, 2023 09:28
@gzdunek gzdunek added this pull request to the merge queue Apr 14, 2023
Merged via the queue into master with commit 4b31ba8 Apr 14, 2023
@gzdunek gzdunek deleted the gzdunek/create-ssh-document-without-command-launcher branch April 14, 2023 10:02
ravicious pushed a commit that referenced this pull request May 23, 2023
* Create dedicated functions for connecting to resources

* Do not use spread operator

* Rename `connectToNode` -> `connectToServer`

* Remove unused imports
ravicious pushed a commit that referenced this pull request May 23, 2023
* Create dedicated functions for connecting to resources

* Do not use spread operator

* Rename `params` -> `telemetry`

* Rename `connectToNode` -> `connectToServer`

* Remove unused imports
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.

3 participants