-
Notifications
You must be signed in to change notification settings - Fork 746
feat(amazonq): update lsp clientname to support sagemaker unified studio case #7817
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
feat(amazonq): update lsp clientname to support sagemaker unified studio case #7817
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
Close and open PR to retrigger CI runs |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
|
✅ I finished the code review, and didn't find any security or code quality issues. |
…o and fix unit tests - Revert the revert of LSP client name update for SageMaker Unified Studio - Fix unit tests to use sinon stubs instead of directly modifying process.env - Consolidate test cases for better maintainability
e800840 to
8b92f76
Compare
…or agentic chat (#7857) ## Problem In order to set appropriate Origin Info on LSP side for SMAI CodeEditor, [link](https://github.com/aws/language-servers/blob/68adf18d7ec46a7ecf9c66fd9d52b1b8f7bc236e/server/aws-lsp-codewhisperer/src/shared/utils.ts#L377), clientInfo needs to be distinct - related [PR to support SMUS](#7817) ## Solution - To check if the environment is SageMaker and a Unified Studio instance and set corresponding clientInfo Name which is ```AmazonQ-For-SMUS-CE``` ## Testing - Built artefact locally using ```npm run compile && npm run package``` and tested on a SMAI CE space - Ran ```npm run test -w packages/toolkit``` which succeeded - LSP logs show the respective client Info details ``` --- - 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: Laxman Reddy <[email protected]>
…or agentic chat (aws#7857) ## Problem In order to set appropriate Origin Info on LSP side for SMAI CodeEditor, [link](https://github.com/aws/language-servers/blob/68adf18d7ec46a7ecf9c66fd9d52b1b8f7bc236e/server/aws-lsp-codewhisperer/src/shared/utils.ts#L377), clientInfo needs to be distinct - related [PR to support SMUS](aws#7817) ## Solution - To check if the environment is SageMaker and a Unified Studio instance and set corresponding clientInfo Name which is ```AmazonQ-For-SMUS-CE``` ## Testing - Built artefact locally using ```npm run compile && npm run package``` and tested on a SMAI CE space - Ran ```npm run test -w packages/toolkit``` which succeeded - LSP logs show the respective client Info details ``` --- - 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: Laxman Reddy <[email protected]>
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 instancesSERVICE_NAMEbeing modified directly from test code causing failure in other tests which identified the environment to be SMUS without clean isolationSolution
AmazonQ-For-SMUS-CETesting
npm run compile && npm run packageand tested on a SMUS CE spacenpm run test -w packages/toolkitwhich succeededAs 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.