Check compatibility when autostarting, not before autostart #32638
Conversation
|
I tested this using packaged versions with my Cloud cluster by manipulating |
Should we change "Failed to verify agent binary" to "Failed to check agent version"? For the EDIT: Or maybe not, it could be confusing. |
| @@ -49,7 +49,7 @@ export function checkAgentCompatibility( | |||
| : 'incompatible'; | |||
| } | |||
|
|
|||
| export function CompatibilityError(): JSX.Element { | |||
| export function CompatibilityError(props: { showAlert: boolean }): JSX.Element { | |||
There was a problem hiding this comment.
hideAlert?: boolean maybe? Usually we want to show the alert, so perhaps it is better to crate a special case for hiding it?
There was a problem hiding this comment.
Yeah, I think I'm going to do hideAlert with a default to false.
| } catch (error) { | ||
| // Turn off autostart if it fails, otherwise the user wouldn't be able to turn it off by | ||
| // themselves. | ||
| workspacesService.setConnectMyComputerAutoStart( | ||
| rootClusterUri, | ||
| false | ||
| ); | ||
| } |
Right, the biggest hurdle is that it's not only checking the version but also downloading it. |
ravicious
force-pushed
the
ravicious/agent-compatibility-fixes
branch
from
2b72f0a to
c5f76e2
Compare
Responding to this comment: #32477 (comment)
The check for
!isAgentConfiguredwas was added here: #31951 (comment)I originally wanted to make it so that if the agent is not started and there's a compatibility error, we show an error indicator. However, the indicator shouldn't be shown if the agent is not configured. Otherwise we'd show the error if versions are incompatible even if the user never touched Connect My Computer.
Instead of adding
isAgentConfiguredinsidecase 'not-started', I assumed that there's no reason to show the indicator at all if the agent is not configured, hence this guard clause.What we really want to ask is "Was the agent not autostarted because of a compatibility error?". But the check for compatibility is done from outside of any
useAsynccall, so there's noattemptobject we could inspect.I was going to say that this isn't necessarily bad and that if
startAgententered an error state because of compatibility issues, we'd have to add a special check for that as well. But is it actually true?Wouldn't it make the code less complex if
shouldAutoStartAgentdidn't look at agent compatibility, merely check if the compatibility is known, and thenstartAgentwould return an error if the versions were incompatible?This PR aims to implement this.