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.

Sign up for GitHub

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

Connect My Computer: Keeping compatibility promise #31951

Merged
merged 18 commits into from
Sep 21, 2023
Merged

Connect My Computer: Keeping compatibility promise #31951

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1d75e91
Add server version to cluster
gzdunek Sep 15, 2023
d90a3b6
Add components to display compatibility promise
gzdunek Sep 15, 2023
f0a570b
Show compatibility promise on status page
gzdunek Sep 15, 2023
bfacfa0
Show compatibility promise on setup page
gzdunek Sep 15, 2023
7b50dad
Rename `serverVersion` -> `proxyVersion`, make all places to use `mak…
gzdunek Sep 20, 2023
29c16c5
Move `UpgradeAgentSuggestion` to a new file, make it stateless
gzdunek Sep 20, 2023
c5c7755
Return `isAgentCompatible` instead of `isNonCompatibleAgent` from con…
gzdunek Sep 20, 2023
3c5823f
Add // DELETE IN comments
gzdunek Sep 20, 2023
8f0b2d4
Improve copies
gzdunek Sep 20, 2023
c4d4744
Add a story for too old client in Setup
gzdunek Sep 20, 2023
1a8f4b1
Extract CONNECT_MY_COMPUTER_RELEASE_MAJOR_VERSION
gzdunek Sep 20, 2023
5312c24
Run prettier
gzdunek Sep 20, 2023
b97a21f
Fix license
gzdunek Sep 20, 2023
02d3218
Show an error on the CMC icon when the agent is not compatible
gzdunek Sep 20, 2023
4983018
Always say "version" before the version number
gzdunek Sep 20, 2023
a4d609b
Adjust tests
gzdunek Sep 20, 2023
3dd2c53
Drop "if you wish" from the copies
gzdunek Sep 20, 2023
2f78da2
Merge branch 'master' into gzdunek/cmc-compatibility-promise
gzdunek Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...rc/ui/ConnectMyComputer/DocumentConnectMyComputerSetup/DocumentConnectMyComputerSetup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ export function DocumentConnectMyComputerSetup() {
function Information(props: { onSetUpAgentClick(): void }) {
const { systemUsername, hostname, roleName, clusterName } =
useAgentProperties();
const { isNonCompatibleAgent } = useConnectMyComputerContext();
const { isAgentCompatible } = useConnectMyComputerContext();

return (
<>
{isNonCompatibleAgent && (
{!isAgentCompatible && (
<>
<CompatibilityError />
<Separator mt={3} mb={2} />
Expand All @@ -93,7 +93,7 @@ function Information(props: { onSetUpAgentClick(): void }) {
css={`
display: block;
`}
disabled={isNonCompatibleAgent}
disabled={!isAgentCompatible}
onClick={props.onSetUpAgentClick}
>
Connect
Expand Down
4 changes: 2 additions & 2 deletions .../ui/ConnectMyComputer/DocumentConnectMyComputerStatus/DocumentConnectMyComputerStatus.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function DocumentConnectMyComputerStatus() {
killAgent,
isAgentConfiguredAttempt,
markAgentAsNotConfigured,
isNonCompatibleAgent,
isAgentCompatible,
} = useConnectMyComputerContext();
const { roleName, systemUsername, hostname } = useAgentProperties();
const { proxyVersion, appVersion, isLocalBuild } = useVersions();
Expand Down Expand Up @@ -205,7 +205,7 @@ export function DocumentConnectMyComputerStatus() {
)}
{prettyCurrentAction.logs && <Logs logs={prettyCurrentAction.logs} />}

{isNonCompatibleAgent ? (
{!isAgentCompatible ? (
<CompatibilityError />
) : (
<>
Expand Down
14 changes: 7 additions & 7 deletions web/packages/teleterm/src/ui/ConnectMyComputer/connectMyComputerContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import { assertUnreachable } from '../utils';

import { hasConnectMyComputerPermissions } from './permissions';

import { isAgentCompatible } from './CompatibilityPromise';
import { isAgentCompatible as checkIfAgentIsComptabile } from './CompatibilityPromise';

import type {
AgentProcessState,
Expand Down Expand Up @@ -82,7 +82,7 @@ export interface ConnectMyComputerContext {
isAgentConfiguredAttempt: Attempt<boolean>;
markAgentAsConfigured(): void;
markAgentAsNotConfigured(): void;
isNonCompatibleAgent: boolean;
isAgentCompatible: boolean;
}

const ConnectMyComputerContext = createContext<ConnectMyComputerContext>(null);
Expand Down Expand Up @@ -129,9 +129,9 @@ export const ConnectMyComputerContextProvider: FC<{
// https://github.com/gravitational/teleport/blob/master/rfd/0133-connect-my-computer.md#access-to-ui-and-autostart
return isFeatureFlagEnabled && (hasPermissions || isAgentConfigured);
}, [configService, isAgentConfigured, mainProcessClient, rootCluster]);
const isNonCompatibleAgent = useMemo(
const isAgentCompatible = useMemo(
() =>
!isAgentCompatible(rootCluster.proxyVersion, mainProcessClient.getRuntimeSettings()),
checkIfAgentIsComptabile(rootCluster.proxyVersion, mainProcessClient.getRuntimeSettings()),
[mainProcessClient, rootCluster]
);

Expand Down Expand Up @@ -283,7 +283,7 @@ export const ConnectMyComputerContextProvider: FC<{
const shouldAutoStartAgent =
isAgentConfigured &&
canUse &&
!isNonCompatibleAgent &&
isAgentCompatible &&
Copy link
Member

Choose a reason for hiding this comment

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

This made me realize: if the agent has been configured but it wasn't started because it's no longer compatible with the proxy, maybe we could display a warning indicator in the icon?

It should be a matter of doing this but please double check this:

diff --git a/web/packages/teleterm/src/ui/ConnectMyComputer/NavigationMenu.tsx b/web/packages/teleterm/src/ui/ConnectMyComputer/NavigationMenu.tsx
index ac8405ceda..25d92d7d55 100644
--- a/web/packages/teleterm/src/ui/ConnectMyComputer/NavigationMenu.tsx
+++ b/web/packages/teleterm/src/ui/ConnectMyComputer/NavigationMenu.tsx
@@ -112,15 +112,28 @@ export function NavigationMenu() {
 
 function getIndicatorStatus(
   currentAction: CurrentAction,
-  isAgentConfiguredAttempt: Attempt<boolean>
+  isAgentConfiguredAttempt: Attempt<boolean>,
+  isAgentCompatible: boolean
 ): IndicatorStatus {
   if (isAgentConfiguredAttempt.status === 'error') {
     return 'error';
   }
 
+  const isAgentConfigured =
+    isAgentConfiguredAttempt.status === 'success' &&
+    isAgentConfiguredAttempt.data;
+
+  if (!isAgentConfigured) {
+    return '';
+  }
+
   if (currentAction.kind === 'observe-process') {
     switch (currentAction.agentProcessState.status) {
       case 'not-started': {
+        if (!isAgentCompatible) {
+          return 'error';
+        }
+
         return '';
       }
       case 'error': {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! We should definitely show it on the icon.

Copy link
Member

Choose a reason for hiding this comment

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

I realized that in an ideal world, the error indicator would only show up if autostart was prevented due to incompatibility issues. Otherwise if you configure the agent, stop it and compatibility issues occur in the future, you're going to see the error indicator even though you're not actively using the agent.

Anyways, I think we can keep it as it is, at least it'll nudge those users towards an upgrade of Connect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be better, but IMO it's still not bad, at least users will be more aware that they are using an incompatible version.

Copy link
Member

Choose a reason for hiding this comment

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

I ran into another issue with the indicator.

isAgentCompatible looks at rootCluster.proxyVersion, but this information is not immediately available. So if I configure my agent and then restart the app, the indicator will show a warning color until the app fetches detailed information about the cluster.

I think we should be able to fix this by replacing isAgentCompatible with agentCompatibility that can be one of three values: compatible, incompatible, unknown. I'll attempt to fix it next week.

Also, during the review I forgot that differentiating between success & error states with just color is usually not enough for users with red-green confusion.

Screen.Recording.2023-09-22.at.15.57.05.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't think of it 😞

I think we should be able to fix this by replacing isAgentCompatible with agentCompatibility that can be one of three values: compatible, incompatible, unknown.

👍

As for colors, probably we should use an icon? I can take a look at it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, an icon would be a good solution.

workspacesService.getConnectMyComputerAutoStart(props.rootClusterUri) &&
agentIsNotStarted;
if (shouldAutoStartAgent) {
Expand All @@ -296,7 +296,7 @@ export const ConnectMyComputerContextProvider: FC<{
isAgentConfigured,
props.rootClusterUri,
workspacesService,
isNonCompatibleAgent,
isAgentCompatible,
]);

return (
Expand All @@ -315,7 +315,7 @@ export const ConnectMyComputerContextProvider: FC<{
markAgentAsConfigured,
markAgentAsNotConfigured,
isAgentConfiguredAttempt,
isNonCompatibleAgent,
isAgentCompatible,
}}
children={props.children}
/>
Expand Down