-
Notifications
You must be signed in to change notification settings - Fork 743
fix(amazonq): update LSP client info name for sagemaker unified studio #7786
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
sdharani91
reviewed
Jul 30, 2025
| * Returns default value of vscode appName or AmazonQ-For-SMUS-CE in case of a sagemaker unified studio environment | ||
| */ | ||
| export function getClientName() { | ||
| if (isSageMaker() && isSageMakerUnifiedStudio()) { |
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.
- Why do we need both these conditions?
- Note that in flare code, the clientInfo check is for a top level field. So we will need to modify that to also check for clientInfo under aws in initialializationOptions.
Contributor
Author
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.
isSageMakerlogic is such that it returns true for a SageMaker environment, hence it will be true for both SMAI and SMUS case, added additional check for SMUS to not affect SMAI- Ack, will make the change to also refer the
clientInfounderawsfield
Contributor
Author
|
Test Failures above observed in |
sdharani91
reviewed
Aug 1, 2025
sdharani91
approved these changes
Aug 1, 2025
laileni-aws
reviewed
Aug 1, 2025
laileni-aws
approved these changes
Aug 2, 2025
chungjac
added a commit
that referenced
this pull request
Aug 5, 2025
…d studio #7786 (#7813) Reverts #7786 This revert PR will fix the failing tests here: https://d1ihu6zq92vp9p.cloudfront.net/c6d9931c-1deb-46ae-ae59-a3d205835a39/report.html Linux Unit Tests are passing 4252/4252: https://d1ihu6zq92vp9p.cloudfront.net/e705989e-fe51-45e4-9772-7ed2ef7eb3fb/report.html
parameja1
added a commit
to parameja1/aws-toolkit-vscode
that referenced
this pull request
Aug 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
In order to set appropriate Origin Info on LSP side for SMUS CodeEditor, link, clientInfo needs to be distinct and with current logic that uses
vscode.env.appNamewhose value will be same for all sagemaker codeeditor instancesSolution
Testing
npm run compile && npm run packageand tested on a SMUS CE spaceAs observed below, the sign out was only disabled for SMUS case initially with this change, a CR followed up which overrode the logic in isSageMaker and returned true for all cases irrespective of the appName passed
SMUS
SMAI
feature/xbranches will not be squash-merged at release time.