Skip to content

Discover: Add UI for connection test for Connect My Computer#35131

Merged
ravicious merged 7 commits intomasterfrom
ravicious/conn-test-feature-flag
Nov 30, 2023
Merged

Discover: Add UI for connection test for Connect My Computer#35131
ravicious merged 7 commits intomasterfrom
ravicious/conn-test-feature-flag

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Nov 29, 2023

cmc-conn-test

This PR adds just the UI for a proper connection test that actually attempts to connect to the node and shows diagnostic messages. The diagnostic messages are yet to be customized towards Connect My Computer (#32206), for now the test behaves the same way as if we were testing a regular SSH node.

I also added a feature flag so that I don't have to wait with merging this PR before I have the backend changes ready.

The implementation is done in a few simple steps:

  1. Take the existing test step, rename it to LegacyTestConnection.
  2. Add a new step in its place with the contents from Server/TestConnection.
  3. Take parts related to Connect My Computer from LegacyTestConnection and fit them into the new component copied from Server.

I had to balance Figma designs with how the existing implementation looks like, both for Connect My Computer and a regular server. Mostly this meant keeping step numbers on the same line as descriptions and how particular buttons and actions are called.

To test this locally, you need to enable the feature flag:

localStorage.setItem('grv_teleport_discover_connect_my_computer_new_connection_test_enabled', 'true')

@ravicious ravicious added backport/branch/v14 no-changelog Indicates that a PR does not require a changelog entry labels Nov 29, 2023
title: 'Test Connection',
component: TestConnection,
eventName: DiscoverEvent.TestConnection,
// TODO(ravicious): Manually emit success event on test connection success.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

useConnectionDiagnostic takes care of this, I didn't have to add any special code to handle this.

Comment on lines +122 to +141
function startSshSession(login: string) {
const url = cfg.getSshConnectRoute({
clusterId: node.clusterId,
serverId: node.id,
login,
});

openNewTab(url);
}

function testConnection(login: string, mfaResponse?: MfaAuthnResponse) {
return runConnectionDiagnostic(
{
resourceKind: 'node',
resourceName: props.agentMeta.resourceName,
sshPrincipal: login,
},
mfaResponse
);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In #34952 I mentioned sharing these two functions between Connect My Computer and Server connection tests, but I think for now I'm going to keep them this way. It's not much code, in the end it calls functions from useConnectionDiagnostic which is already shared. And to make the diagnostic messages be more relevant to Connect My Computer, I'll have to add some extra parameter to runConnectionDiagnostic anyway.

So in the end I don't think there's much to be gained from sharing these two functions.

'foo',
'bar',
'baz',
'czesława_maria_de_domo_cieślak_primo_voto_gospodarek_secundo_voto_kowalczyk',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Server/TestConnection uses a shorter login:

node.sshLogins = [
...node.sshLogins,
'george_washington_really_long_name_testing',
];

and the width of the login select is set to be just wide enough to contain it, but it fails with the full name of Violetta Villas. 😭

login-select-with-long-values

For now I'm just not going to try to fix what's broken as I couldn't quickly figure out how to make react-select work in this scenario.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from gzdunek November 30, 2023 07:43
@gzdunek gzdunek self-requested a review November 30, 2023 09:11
buttonOnClick: () => void;
}) => (
<StepSkeletonPickUser>
<>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unnecessary <>.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There was a few more unnecessary fragments in this file. react/jsx-no-useless-fragment doesn't catch that, even with allowExpressions disabled.

@ravicious ravicious enabled auto-merge November 30, 2023 09:53
@ravicious ravicious added this pull request to the merge queue Nov 30, 2023
Merged via the queue into master with commit a7d95b7 Nov 30, 2023
@ravicious ravicious deleted the ravicious/conn-test-feature-flag branch November 30, 2023 10:08
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v14 Failed

@ravicious
Copy link
Copy Markdown
Member Author

DISCOVER_CONNECT_MY_COMPUTER_NEW_CONNECTION_TEST_ENABABLED
ENABABLED

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/lg ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants