feat(session): add SageMaker Unified Studio portal redirect #106
Conversation
| ResourceArn?: string | ||
| ResourceName?: string | ||
| AppImageVersion?: string | ||
| AdditionalMetadata?: { |
There was a problem hiding this comment.
interesting, I'm actually not able to see that, can you please add more details on where exactly you are seeing it
There was a problem hiding this comment.
Nit: Was using more than tab (4 space). Hence called it.
| const sagemakerCookie: SagemakerCookie = r as SagemakerCookie | ||
|
|
||
| initialize(sagemakerCookie); | ||
| updateStatusItemWithMetadata(context); |
There was a problem hiding this comment.
Any issues with SM AI if we move this out of this code block scope?
There was a problem hiding this comment.
this should be okay, but I would rather set the smusRedirectUrl seperately from updateStatusItemWithMetadata, I feel like we're overloading this func. Refactor fecthing jsonData into a helper function
There was a problem hiding this comment.
Thanks for the suggestion, I am adding helper function in next revision.
| sagemaker-idle-extension.patch | ||
| terminal-crash-mitigation.patch | ||
| sagemaker-open-notebook-extension.patch | ||
| sagemaker-extension-smus-support.patch |
There was a problem hiding this comment.
Any strong reason why not have it in sagemaker-extension.diff itself and have it in separate patch?
There was a problem hiding this comment.
patches are feature based
| DataZoneDomainRegion: !!DataZoneDomainRegion, | ||
| DataZoneProjectId: !!DataZoneProjectId | ||
| }); | ||
| return null; |
There was a problem hiding this comment.
What's the SMUS session renewal UX if we're unable to generate the url?
There was a problem hiding this comment.
If SMUS URL is not generated then User get redirected to AWS Console sign in page (default Redirect URL return by LL). Basically user would need to manually generate VSCode link from SMUS portal. Ideally we should have monitoring to detect such cases in which we can't generate URLs.
| * Constructs the SMUS portal URL using domain, region, and project information | ||
| * Returns null if not in SMUS environment or if required fields are missing | ||
| */ | ||
| export const getSmusVscodePortalUrl = (metadata: SagemakerMetadata): string | null => { |
There was a problem hiding this comment.
why can't we follow existing pattern of setting redirect url in cookies?
There was a problem hiding this comment.
Redirect url is set by LooseLeaf, do you mean LL return Redirect URL of SMUS for SMUS use cases?
There was a problem hiding this comment.
Thank for setting "Redirect URL" suggestion from LooseLeaf side. This looks like a better long term solution. It will need to good amount of research to make sure it doesn't impact any other functionality and also "Redirect URL" is generic solution for all IDEs so updating it could have impact on other IDEs in unexpected way. I'll suggest we do it in incremental way. First we use current short term way (this PR) and then we see if we can get "Redirect URL" updated from LL side as long term way.
There was a problem hiding this comment.
sounds good, please create a tracking issue for this.
There was a problem hiding this comment.
| sagemaker-idle-extension.patch | ||
| terminal-crash-mitigation.patch | ||
| sagemaker-open-notebook-extension.patch | ||
| sagemaker-extension-smus-support.patch |
There was a problem hiding this comment.
patches are feature based
arunbhati
force-pushed
the
feature/smus-sagemaker-extension
branch
from
097a7ba to
965cfe3
Compare
** Description ** • Add support for redirecting to SMUS portal during VSCode session renewal • Detect SMUS environment using SERVICE_NAME variable • Update metadata handling to support SMUS portal URL construction ** Motivation ** • Enable users to renew VSCode sessions through SMUS portal • Improve session renewal experience for SMUS users ** Testing ** • Verified with local VSCode server • Tested for both SMUS and SageMaker AI with manually cookie population and metadata file creation ** Backward compatibility ** • Maintains existing behavior for non-SMUS environments • Silent failure handling prevents disruption of current flows
arunbhati
force-pushed
the
feature/smus-sagemaker-extension
branch
from
965cfe3 to
3983b4d
Compare
| return JSON.parse(data) as SagemakerResourceMetadata; | ||
| } catch (error) { | ||
| // fail silently not to block users | ||
| console.error('Error reading metadata file:', error); |
There was a problem hiding this comment.
Non-blocking: we should move all these logs to output channel. like #110 (comment)
| ResourceArn?: string | ||
| ResourceName?: string | ||
| AppImageVersion?: string | ||
| AdditionalMetadata?: { |
There was a problem hiding this comment.
Nit: Was using more than tab (4 space). Hence called it.
…s-sagemaker-extension
…s-sagemaker-extension
…nsion feat(session): add SageMaker Unified Studio portal redirect
3 participants
DESCRIPTION
• Add support for redirecting to SMUS portal during VSCode session renewal • Detect SMUS environment using SERVICE_NAME variable • Update metadata handling to support SMUS portal URL construction
MOTIVATION
• Enable users to renew VSCode sessions through SMUS portal • Improve session renewal experience for SMUS users
TESTING
• Verified with local VSCode server
• Tested for both SMUS and SageMaker AI with manually cookie population and metadata file creation
BACKWARD COMPATIBILITY
• Maintains existing behavior for non-SMUS environments • Silent failure handling prevents disruption of current flows
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.