-
Notifications
You must be signed in to change notification settings - Fork 730
feat(sagemakerunifiedstudio): Support for SageMaker Unified Studio #7991
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.
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
Conversation
## Problem - DeepLink remote connections should be able to reconnect automatically when the connection drops. ## Solution - Reintroduced and updated logic to handle DeepLink reconnection by redirecting the user to the Studio UI to refetch the session token. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
## Problem
When persisting selected domains/users that the customer manually
filtered, we did not take into account the region that the customer was
operating in. Thus, the filtering mechanism was incorrectly being
applied to all regions.
Example:
```
[
[
'arn:aws:iam:user/user1',
['domain1__pdx-1', 'domain1__pdx-2']
],
]
```
## Solution
When persisting the selected domains/users in global state, we insert
another level to track region. Example:
```
[
[
'us-west-2',
[
[
'arn:aws:iam:user/user1',
['domain1__pdx-1', 'domain1__pdx-2']
]
]
],
[
'us-east-1',
[
[
'arn:aws:iam:user/user1',
['domain1__iad-1', 'domain1__iad-2']
]
]
]
]
```
---
- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).
- License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.
---------
Co-authored-by: Newton Der <[email protected]>
| } | ||
| } | ||
|
|
||
| if (value === Constants.LOCAL_PYTHON || value === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Scope for refactoring not a blocker though
const shouldResetProject = value !== previousConnection || !state.project
if (shouldResetProject) {
state.project = undefined
if (value === Constants.LOCAL_PYTHON || (value && value !== previousConnection)) {
this.setDefaultProjectForConnection(cell, value)
}
}
| if (hasLocalMagic) { | ||
| const magicCommand = this.getMagicCommand(cell) | ||
| if (magicCommand) { | ||
| if (isMagicCommand) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Avoid nested if
| : [createPlaceholderItem(NO_DATA_FOUND_MESSAGE) as RedshiftNode] | ||
| } catch (err) { | ||
| logger.error(`Failed to get columns: ${(err as Error).message}`) | ||
| const errorMessage = (err as Error).message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This variable can be declared at L699 instead
| smusConnectionType: connection.type, | ||
| smusProjectRegion: connection.location?.awsRegion, | ||
| }) | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is code duplication in this try catch block
- Extract helper functions.
| return false | ||
| } | ||
|
|
||
| export function getFormattedDateTime(utcString: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be moved to shared/utils
| return formatted | ||
| } | ||
|
|
||
| export function getFormattedCurrentTime(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| @@ -0,0 +1 @@ | |||
| # Common business logic and APIs for SageMaker Unified Studio features | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| export const errorCode = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move this to shared/errors may be
laileni-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address comments in followup PR's
Thanks
7872634 to
0a8b981
Compare
* SMUS needs to be activated in NodeJS environment only. * Remove Notebook resources * Added changelog
0a8b981 to
10e40ee
Compare
|
/retryBuilds |
|
/retryBuilds |
1 similar comment
|
/retryBuilds |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
🔴 I'm not able to complete the code review because the diff size exceeds the limits. Consider splitting your changes into smaller diffs and try again. For more information, see Diff limits in the GitHub documentation Request ID : 1d0618bd-8f3a-487a-8f73-c4d99dfa555d |
|
/retryBuild |
Problem
Access SMUS from IDE.
Solution
Support IDE support for Unified Studio
feature/xbranches will not be squash-merged at release time.