fix(sagemaker): improve error handling when the Space SSH host check fails #8287
Conversation
bhavya2109sharma
changed the title
bhavya2109sharma
changed the title
|
bhavya2109sharma
changed the title
vpbhargav
left a comment
vpbhargav
left a comment
There was a problem hiding this comment.
I think worth testing this change in Widows/Mac and Linux if possible. Hard to validate regex validations and some of these platforms have casing, newline peculiarities.
bhavya2109sharma
force-pushed
the
fix/SSH-host
branch
from
d4c4eb1 to
078ca7a
Compare
bhavya2109sharma
force-pushed
the
fix/SSH-host
branch
from
1937081 to
467e065
Compare
|
|
||
| // Check against known versions | ||
| const knownVersions = [ | ||
| this.createSSHConfigSection(proxyCommand).trim(), // Current version |
There was a problem hiding this comment.
Not necessary, but something to think about: Instead of generating all possible config values (every time), you could store a list of functions here. Instead of iterating the values, you iterate the functions, and for each one, you call the function and then check for the result in the config. If found, then you shouldn't need to proceed to the next function, avoiding generating additional strings. This isn't necessary in this case because these strings are relatively short, and there won't be very many, so the performance impact is negligible, but it's worth being aware of this "lazy" pattern for future situations.
| let startIndex = -1 | ||
| let endIndex = -1 |
There was a problem hiding this comment.
Why is it necessary to use these pointers? This feels over-complicated. If we stick to exact string matching (using the list of past expected sm_* sections), we can easily determine the start and end indices if we need to make a replacement.
const startIndex = configContent.indexOf(pastSection)
if (startIndex == -1) {
// Not found
}
const endIndex = startIndex + pastSection.length
// Then slice and generate the new content stringWe can probably simplify even further, as I don't imagine we need the indices.
if (configContent.includes(pastSection)) {
const newContent = configContent.replace(pastSection, newSection)
// Then write newContent to the file
}3 participants
Problem
When attempting to connect to a SageMaker Space, if the user's SSH config had issues (like the old
User '%r'directive),users would see an unhelpful error :
This happened because:
Solution
Implemented a proactive SSH config validation that checks for outdated config before running SSH validation
Prompts users to update outdated config automatically
Provides helpful error messages with line numbers for external errors
Validates before showing progress dialogs (better UX)
Changes Made
New Files Created
packages/core/src/awsService/sagemaker/sshConfig.ts: CreatedSageMakerSshConfigclass that extends the baseSshConfigclass with SageMaker specific SSH logic.packages/core/src/test/awsService/sagemaker/sshConfig.test.ts- test suiteModified
packages/core/src/awsService/sagemaker/model.ts: RemovedensureValid()fromprepareDevEnvConnection()and added comment explaining validation happens incommands.tspackages/core/src/awsService/sagemaker/commands.ts: AddedvalidateSshConfig()which will validate before showing progress dialogs so that users can fix issues before any connection attempt startsTesting
User '%r')feature/xbranches will not be squash-merged at release time.