Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

Display cluster name for each connection & add autoconnect #678

Merged
gzdunek merged 4 commits intomasterfrom
gzdunek/connections-improvements
Mar 18, 2022
Merged

Display cluster name for each connection & add autoconnect #678
gzdunek merged 4 commits intomasterfrom
gzdunek/connections-improvements

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Mar 17, 2022

Also fixes a bug when connecting to server in a leaf cluster

@gzdunek gzdunek requested a review from ravicious March 17, 2022 17:45
createGateway();
}
}, [cluster.connected]);

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.

What kind of scenario is this + the change in the reconnect above are going to handle?

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Mar 18, 2022

Choose a reason for hiding this comment

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

When you click on a connection it opens new tab with text: Click to reconnect, even when you are connected to the cluster (and in useReconnect it just changes document status).
After this change, clicking on a connection will try to make the connection automatically and open a tab.

ctx.clustersService.findRootClusterByResource(serverUri);
const documentsService = ctx.workspacesService.getWorkspaceDocumentService(
cluster.uri
rootCluster.uri
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.

Yeah, I'm still kinda conflicted whether we should convert a resource URI to a root cluster URI this way. The root cluster URI seems to be baked into every other URI, so theoretically we should be able to get it just from operations on that string alone, without having to use clustersService. My main concern is that in other places it'll require pulling in that clustersService which can unnecessarily complicate tests.

We might think about something like routing.extractRootClusterUri in the future. For now I added just ensureRootClusterUri there which takes any kind of cluster URI and converts it to a root cluster URI.

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.

Good point, I will add it to our master ticket

@gzdunek gzdunek force-pushed the gzdunek/pickers-empty-state branch 3 times, most recently from faad431 to 49e615d Compare March 18, 2022 10:55
@gzdunek gzdunek force-pushed the gzdunek/connections-improvements branch from 0194935 to ee326da Compare March 18, 2022 11:08
@gzdunek gzdunek changed the base branch from gzdunek/pickers-empty-state to master March 18, 2022 11:09
@gzdunek gzdunek force-pushed the gzdunek/connections-improvements branch from ee326da to 15e9f2e Compare March 18, 2022 11:29
@gzdunek gzdunek force-pushed the gzdunek/connections-improvements branch from 15e9f2e to f07ad95 Compare March 18, 2022 11:40
@gzdunek gzdunek merged commit 46e9c3e into master Mar 18, 2022
@gzdunek gzdunek deleted the gzdunek/connections-improvements branch March 18, 2022 11:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants