SAM: change config to "machine" scope; handle failed config write #1241

justinmk3 · 2020-08-14T05:57:48Z

Using Global scope for the aws.samcli.location config entry is untenable because:

Multiple running VSCode instances may change it, and then all instances respond to the onDidChangeConfiguration event, which may cause hysterisis (on/off loop).
Remote VSCode instances share Global config with local running VSCode instances. Thus machine-dependent settings must not be Global, because the config value on a remote VSCode may conflict with local instance(s).

Workspace is a reasonable scope for aws.samcli.location because it is usually auto-detected (setting it manually is usually only for debugging/experimentation)

Note

From the vscode settings UI one can see that there is a concept of machine-specific configuration: ~/.vscode-server/data/Machine/settings.json . But this is not exposed in the vscode API.
- Update: However, we can set scope=machine in the package.json declaration. See 3f6b1f3
We could greatly simplify the code, and avoid most of these problems, by simply not updating the user's config. If the user's configured SAM CLI is invalid, do auto-detection just-in-time.
- Done in PR: SAM CLI detection: do not auto-update user setting #1242
Related upstream issue (fixed Dec 2020): "machine-scope setting does not get updated when remote vscode instance sets it globally" Update remote settings when a setting from an extension that is running remotely is changed microsoft/vscode-remote-release#1801

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

src/test/shared/sam/cli/samCliConfiguration.test.ts

awschristou · 2020-08-14T16:31:49Z

src/shared/sam/cli/samCliConfiguration.ts

@@ -55,8 +55,12 @@ export class DefaultSamCliConfiguration implements SamCliConfiguration {
        const detectedLocation = (await this._samCliLocationProvider.getLocation()) ?? ''
        // Avoid setting the value redundantly (could cause a loop because we
        // listen to the `onDidChangeConfiguration` event).
-        if (configLocation !== detectedLocation) {
-            await this.setSamCliLocation(detectedLocation)
+        if (detectedLocation && configLocation !== detectedLocation) {


If SAM CLI is no longer detected, why aren't we writing this to the config? Seems like the toolkit would incorrectly think the CLI is at some location.

I don't see any callsites where that would matter. Calls to sam will fail in any case.

But #1242 is a more complete solution that updates all call sites to explicitly deal with just-in-time autodetection instead of doing contortions to update the user's config (which is not actually necessary and a bit of an anti-pattern).

It could be misleading in that you've run the "detect sam cli" command, and the toolkit shows the location configured as the old path (instead of no path). I'm fine proceeding with this in anticipation of 1242 then.

Using `Global` scope for the `aws.samcli.location` config entry is untenable because: - Multiple running VSCode instances may change it, and then all instances respond to the `onDidChangeConfiguration` event, which may cause hysterisis (on/off loop). - _Remote_ VSCode instances share `Global` config with local running VSCode instances. Thus machine-dependent settings must not be `Global`, because the config value on a remote VSCode may conflict with local instance(s). `Workspace` is a reasonable scope for `aws.samcli.location` because it is usually auto-detected (setting it manually is usually only for debugging/experimentation). Note: - From the vscode settings UI one can see that there is a concept of machine-specific configuration: ~/.vscode-server/data/Machine/settings.json - But this is not exposed in the vscode API. - But we *can* use scope=machine in the package.json declaration. That is done in the next commit. - We could greatly simplify the code, and avoid most of these problems, by simply _not_ updating the user's config. If the user's configured SAM CLI is invalid, do auto-detection just-in-time...

justinmk3 · 2020-08-15T03:46:59Z

Updated to set scope=machine in package.json .
- This helps, but now VSCode expects the user to enter a value: writeSetting() does not update the machine-local config.
- This at least avoids bad interactions with Remote instances.
- scope=machine has a known bug (closed but NOT resolved): WokspaceConfiguration.update() writes machine setting to user settings.json microsoft/vscode#102543
- Long-term solution is to use just-in-time detection if the user config value is invalid: SAM CLI detection: do not auto-update user setting #1242
~~This also appears to fix CI: restore Windows CI; use "--verbose --log debug" #1223~~

This PR is a low-risk step that we can take in the meantime.

We can define a config value as "machine" scope. But note the lack of `vscode.ConfigurationTarget.Machine` scope, it is expected that we use `vscode.ConfigurationTarget.Global`. Then VSCode will resolve the config value depending on "User" and "Remote" settings, and depending on whether the current session is a Local or Remote instance. NOTE: - Current behavior of `scope=machine` is such that VSCode expects the user to enter a value: writeSetting() does not update the machine-local config. - `scope=machine` has a known bug (issue is closed but NOT resolved): microsoft/vscode#102543 - Long-term solution is to use a passive warning combined with just-in-time detection, rather than fighting VSCode (and the user) to auto-update a user setting. if the user config value is invalid: #1242 REFERENCE: For reference, `Workspace` and `WorkspaceFolder` scopes require a workspace{folder} to write a setting, else writeSetting() fails: await this._configuration.writeSetting(k, detectedLocation, vscode.ConfigurationTarget.Workspace) 2020-08-15 01:50:45 [ERROR]: failed to set config: 'samcli.location'='/home/linuxbrew/.linuxbrew/bin/sam', error: Error: Unable to write aws.samcli.location to Workspace Settings. This setting can be written only into User settings. at D.reject (file:///…/vs/workbench/workbench.desktop.main.js:5544:245) at D.resolveAndValidate (file:///…/vs/workbench/workbench.desktop.main.js:5547:622) at D.doWriteConfiguration (file:///…/vs/workbench/workbench.desktop.main.js:5541:414) at Object.factory (file:///…/vs/workbench/workbench.desktop.main.js:5541:190) at u.consume (file:///…/vs/workbench/workbench.desktop.main.js:130:421) at file:///…/vs/workbench/workbench.desktop.main.js:130:236 at new Promise (<anonymous>) at u.queue (file:///…/vs/workbench/workbench.desktop.main.js:130:160) at D.writeConfiguration (file:///…/vs/workbench/workbench.desktop.main.js:5541:175) at C.writeConfigurationValue (file:///…/vs/workbench/workbench.desktop.main.js:5569:356) at file:///…/vs/workbench/workbench.desktop.main.js:5557:594 at processTicksAndRejections (internal/process/task_queues.js:85:5) { name: 'Error', message: 'Unable to write aws.samcli.location to Workspace ' + 'Settings. This setting can be written only into User ' + 'settings.' } await this._configuration.writeSetting(k, detectedLocation, vscode.ConfigurationTarget.WorkspaceFolder) 2020-08-15 01:50:45 [ERROR]: failed to set config: 'samcli.location'='/home/linuxbrew/.linuxbrew/bin/sam', error: Error: Unable to write to Folder Settings because no resource is provided. at D.reject (file:///…/vs/workbench/workbench.desktop.main.js:5544:245) at D.resolveAndValidate (file:///…/vs/workbench/workbench.desktop.main.js:5547:673) at D.doWriteConfiguration (file:///…/vs/workbench/workbench.desktop.main.js:5541:414) at Object.factory (file:///…/vs/workbench/workbench.desktop.main.js:5541:190) at u.consume (file:///…/vs/workbench/workbench.desktop.main.js:130:421) at file:///…/vs/workbench/workbench.desktop.main.js:130:236 at new Promise (<anonymous>) at u.queue (file:///…/vs/workbench/workbench.desktop.main.js:130:160) at D.writeConfiguration (file:///…/vs/workbench/workbench.desktop.main.js:5541:175) at C.writeConfigurationValue (file:///…/vs/workbench/workbench.desktop.main.js:5569:356) at file:///…/vs/workbench/workbench.desktop.main.js:5557:594 at processTicksAndRejections (internal/process/task_queues.js:85:5) { name: 'Error', message: 'Unable to write to Folder Settings because no resource is provided.' }

src/shared/settingsConfiguration.ts

awschristou · 2020-08-17T18:28:24Z

src/shared/sam/cli/samCliConfiguration.ts

@@ -55,8 +55,12 @@ export class DefaultSamCliConfiguration implements SamCliConfiguration {
        const detectedLocation = (await this._samCliLocationProvider.getLocation()) ?? ''
        // Avoid setting the value redundantly (could cause a loop because we
        // listen to the `onDidChangeConfiguration` event).
-        if (configLocation !== detectedLocation) {
-            await this.setSamCliLocation(detectedLocation)
+        if (detectedLocation && configLocation !== detectedLocation) {


It could be misleading in that you've run the "detect sam cli" command, and the toolkit shows the location configured as the old path (instead of no path). I'm fine proceeding with this in anticipation of 1242 then.

`vscode.workspace.getConfiguration(…).update(…)` (wrapped by `writeSetting()`) may throw an exception in various conditions, including permissions failure, unmet requirements, etc. This causes: Error Activating AWS Toolkit Toolkit must not throw when `writeSetting()`, it is used in critical phases such as activation.

Instead of eager-detecting SAM CLI and force-updating the user setting, 1. Never force-update the user setting (unless user invokes `AWS: Detect SAM CLI` command). * Eliminates lots of complexity, e.g.: * Because the `aws.samcli.location` config is `Global`, a change in _any_ VSCode instance will trigger `onDidChangeConfiguration` in _all_ running VSCode instances. * VSCode shares settings between local and remote instances. (This may depend on the `remote.SSH.useLocalServer` setting?) 2. At various sites where SAM CLI is needed, auto-detect SAM CLI if and only if the user setting is empty. * We cannot "validate" SAM CLI location because in a remote session, `require('fs').existsSync('/home/linuxbrew/.linuxbrew/bin/sam')` returns false even if the path is valid in WSL. #1300 Followup (and partial revert) of #1241.

Instead of eager-detecting SAM CLI and force-updating the user setting, 1. Never force-update the user setting (unless user invokes `AWS: Detect SAM CLI` command). * Eliminates lots of complexity, e.g.: * Because the `aws.samcli.location` config is `Global`, a change in _any_ VSCode instance will trigger `onDidChangeConfiguration` in _all_ running VSCode instances. * VSCode shares settings between local and remote instances. (This may depend on the `remote.SSH.useLocalServer` setting?) 2. At various sites where SAM CLI is needed, auto-detect SAM CLI if and only if the user setting is empty. * We cannot "validate" SAM CLI location because in a remote session, `require('fs').existsSync('/home/linuxbrew/.linuxbrew/bin/sam')` returns false even if the path is valid in WSL. #1300 TEST CASES - Set `samcli.location` setting to an invalid path. - On startup, AWS Toolkit should _not_ warn about the invalid path. - Invoking `AWS: Detect SAM CLI` _should_ update the setting. - Clear the `samcli.location` setting then restart vscode. - On startup, AWS Toolkit should _not_ validate nor update the `samcli.location` setting. - Invoking `AWS: Detect SAM CLI` _should_ update the setting. Followup (and partial revert) of #1241.

AWSQ: Rename context menu item "Send to CW Chat" to "Send to AWS Q"

justinmk3 requested a review from a team as a code owner August 14, 2020 05:57

justinmk3 force-pushed the jmk-samdetect branch from ed396fc to 6712bf3 Compare August 14, 2020 05:59

justinmk3 mentioned this pull request Aug 14, 2020

SAM CLI detection: do not auto-update user setting #1242

Merged

justinmk3 commented Aug 14, 2020

View reviewed changes

src/test/shared/sam/cli/samCliConfiguration.test.ts Outdated Show resolved Hide resolved

awschristou reviewed Aug 14, 2020

View reviewed changes

justinmk3 force-pushed the jmk-samdetect branch from 28628a1 to 2359134 Compare August 15, 2020 03:41

justinmk3 force-pushed the jmk-samdetect branch from 2359134 to dcf1689 Compare August 15, 2020 04:09

justinmk3 changed the title ~~SAM CLI detection: change to "Workspace" scope~~ SAM CLI detection: change to "machine" scope Aug 15, 2020

justinmk3 changed the title ~~SAM CLI detection: change to "machine" scope~~ SAM CLI config: change to "machine" scope Aug 15, 2020

justinmk3 mentioned this pull request Aug 15, 2020

CI test matrix failures #1207

Closed

justinmk3 commented Aug 17, 2020

View reviewed changes

src/shared/settingsConfiguration.ts Show resolved Hide resolved

justinmk3 changed the title ~~SAM CLI config: change to "machine" scope~~ SAM: change config to "machine" scope; handle failed config write Aug 17, 2020

awschristou reviewed Aug 17, 2020

View reviewed changes

justinmk3 force-pushed the jmk-samdetect branch from dcf1689 to 68d2734 Compare August 17, 2020 20:01

awschristou approved these changes Aug 17, 2020

View reviewed changes

justinmk3 merged commit 6c37bbc into master Aug 17, 2020

justinmk3 deleted the jmk-samdetect branch August 17, 2020 20:15

This was referenced Aug 17, 2020

Error Activating AWS Toolkit #1229

Closed

SAM CLI detection: "Don't show again" if SAM CLI is not found #1244

Closed

justinmk3 mentioned this pull request Sep 16, 2020

SAM debug cannot find SAM CLI in Toolkit 1.13+ (WSL 2) #1300

Closed

justinmk3 mentioned this pull request Mar 30, 2021

doc: project guidelines #1608

Merged

justinmk3 pushed a commit that referenced this pull request Nov 28, 2023

Merge pull request #1241 from aws/ivchea/rename_send_to_chat

d8d89b2

AWSQ: Rename context menu item "Send to CW Chat" to "Send to AWS Q"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SAM: change config to "machine" scope; handle failed config write #1241

SAM: change config to "machine" scope; handle failed config write #1241

justinmk3 commented Aug 14, 2020 •

edited

Loading

awschristou Aug 14, 2020

justinmk3 Aug 14, 2020 •

edited

Loading

awschristou Aug 17, 2020

justinmk3 commented Aug 15, 2020 •

edited

Loading

awschristou Aug 17, 2020

SAM: change config to "machine" scope; handle failed config write #1241

SAM: change config to "machine" scope; handle failed config write #1241

Conversation

justinmk3 commented Aug 14, 2020 • edited Loading

Note

License

awschristou Aug 14, 2020

Choose a reason for hiding this comment

justinmk3 Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

awschristou Aug 17, 2020

Choose a reason for hiding this comment

justinmk3 commented Aug 15, 2020 • edited Loading

awschristou Aug 17, 2020

Choose a reason for hiding this comment

justinmk3 commented Aug 14, 2020 •

edited

Loading

justinmk3 Aug 14, 2020 •

edited

Loading

justinmk3 commented Aug 15, 2020 •

edited

Loading