From 89405f91ec8b7eb45ab80194084bc169098bdd29 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 13 Aug 2020 20:32:20 -0700 Subject: [PATCH] SAM-detect: give priority to user setting 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. --- src/shared/sam/activation.ts | 4 +- src/shared/sam/cli/samCliConfiguration.ts | 55 ++++++++++++------ src/shared/sam/cli/samCliDetection.ts | 56 +++++++++---------- src/shared/sam/cli/samCliInvoker.ts | 23 ++++---- src/shared/sam/cli/samCliLocalInvoke.ts | 10 +++- src/shared/sam/cli/samCliValidator.ts | 15 +++-- src/shared/utilities/childProcess.ts | 2 +- .../sam/cli/samCliConfiguration.test.ts | 26 +-------- .../shared/sam/cli/samCliValidator.test.ts | 10 +++- 9 files changed, 103 insertions(+), 98 deletions(-) diff --git a/src/shared/sam/activation.ts b/src/shared/sam/activation.ts index df51ef53d42..02ecb43f5dd 100644 --- a/src/shared/sam/activation.ts +++ b/src/shared/sam/activation.ts @@ -65,11 +65,11 @@ export async function activate(ctx: ExtContext): Promise { ) ) - await detectSamCli({ passive: true, showMessage: false }) ctx.extensionContext.subscriptions.push( vscode.workspace.onDidChangeConfiguration(configurationChangeEvent => { if (configurationChangeEvent.affectsConfiguration('aws.samcli.location')) { - detectSamCli({ passive: true, showMessage: undefined }) + // This only shows a message (passive=true), does not set anything. + detectSamCli({ passive: true, showMessage: true }) } }) ) diff --git a/src/shared/sam/cli/samCliConfiguration.ts b/src/shared/sam/cli/samCliConfiguration.ts index 9d935e53d7b..72fb10b5727 100644 --- a/src/shared/sam/cli/samCliConfiguration.ts +++ b/src/shared/sam/cli/samCliConfiguration.ts @@ -4,16 +4,31 @@ */ import * as vscode from 'vscode' -import * as filesystemUtilities from '../../filesystemUtilities' import { SettingsConfiguration } from '../../settingsConfiguration' import { SamCliLocationProvider } from './samCliLocator' export interface SamCliConfiguration { + /** Gets the current SAM CLI location from the VSCode settings store. */ getSamCliLocation(): string | undefined + /** Sets the SAM CLI location in the VSCode settings store. */ setSamCliLocation(location: string | undefined): Promise + /** + * Initializes this SamCliConfiguration object from the VSCode user settings, + * or tries to auto-detect `sam` in the environment. + * + * @returns true if auto-detection failed; false if auto-detection succeeded or was not attempted. + */ initialize(): Promise + + /** + * Gets location of `sam` from user config, or tries to find `sam` on the + * system if the user config is invalid. + * + * @returns empty string if `sam` was not found on the system + */ + getOrDetectSamCli(): Promise<{ path: string; autoDetected: boolean }> } export class DefaultSamCliConfiguration implements SamCliConfiguration { @@ -26,12 +41,10 @@ export class DefaultSamCliConfiguration implements SamCliConfiguration { this._samCliLocationProvider = samCliLocationProvider } - /** Gets the current SAM CLI location from the VSCode settings store. */ public getSamCliLocation(): string | undefined { return this._configuration.readSetting(DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION) } - /** Sets the SAM CLI location in the VSCode settings store. */ public async setSamCliLocation(location: string | undefined): Promise { await this._configuration.writeSetting( DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION, @@ -41,22 +54,32 @@ export class DefaultSamCliConfiguration implements SamCliConfiguration { } /** - * Initializes this SamCliConfiguration object from the VSCode user settings, - * or tries to auto-detect `sam` in the environment. + * Gets the `samcli.location` setting if set by the user, else searches for + * `sam` on the system and returns the result. + * + * Returns `autoDetected=true` if auto-detection was _attempted_. If `sam` + * was not found on the system then `path=""`. */ - public async initialize(): Promise { - const configLocation = this.getSamCliLocation() ?? '' - if (configLocation) { - if (await filesystemUtilities.fileExists(configLocation)) { - return - } + public async getOrDetectSamCli(): Promise<{ path: string; autoDetected: boolean }> { + const fromConfig = + this._configuration.readSetting(DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION) ?? '' + // Respect user setting, do not validate it. fileExists() does not + // understand WSL paths, for example. https://github.com/aws/aws-toolkit-vscode/issues/1300 + if (fromConfig) { + return { path: fromConfig, autoDetected: false } } + const fromSearch = (await this._samCliLocationProvider.getLocation()) ?? '' + return { path: fromSearch, autoDetected: true } + } - const detectedLocation = (await this._samCliLocationProvider.getLocation()) ?? '' - // Avoid setting the value redundantly (could cause a loop because we - // listen to the `onDidChangeConfiguration` event). - if (detectedLocation && configLocation !== detectedLocation) { - await this.setSamCliLocation(detectedLocation) + // TODO: remove this, it's only used by tests. Maybe the tests can call + // detectSamCli() instead, or likely the tests need to be revisited entirely. + public async initialize(): Promise { + const samPath = await this.getOrDetectSamCli() + if (!samPath.autoDetected) { + return } + + await this.setSamCliLocation(samPath.path) } } diff --git a/src/shared/sam/cli/samCliDetection.ts b/src/shared/sam/cli/samCliDetection.ts index d74d99d0f63..0f284d98b1d 100644 --- a/src/shared/sam/cli/samCliDetection.ts +++ b/src/shared/sam/cli/samCliDetection.ts @@ -18,17 +18,15 @@ const lock = new AsyncLock() const learnMore = localize('AWS.samcli.userChoice.visit.install.url', 'Get SAM CLI') const browseToSamCli = localize('AWS.samcli.userChoice.browse', 'Locate SAM CLI...') const settingsUpdated = localize('AWS.samcli.detect.settings.updated', 'Settings updated.') -const settingsNotUpdated = localize('AWS.samcli.detect.settings.not.updated', 'No settings changes necessary.') - -/** Most-recent resolved SAM CLI location. */ -let currentsamCliLocation: string | undefined = undefined /** + * Searches for SAM CLI on the system, updates the user setting if + * `passive=false`, and shows a message if `showMessage=true`. * - * @param args.passive If true, this was _not_ a user-initiated action. + * @param args.passive Controls whether user setting is updated. Also sets the + * telemetry "passive" flag. * @param args.showMessage true: always show message, false: never show - * message, undefined: show message only if the new setting differs from - * the old setting. + * message (except if SAM was not found) */ export async function detectSamCli(args: { passive: boolean; showMessage: boolean | undefined }): Promise { await lock.acquire('detect SAM CLI', async () => { @@ -37,28 +35,34 @@ export async function detectSamCli(args: { passive: boolean; showMessage: boolea new DefaultSamCliLocationProvider() ) - const valueBeforeInit = samCliConfig.getSamCliLocation() - await samCliConfig.initialize() - const valueAfterInit = samCliConfig.getSamCliLocation() - const isAutoDetected = valueBeforeInit !== valueAfterInit - const isUserSettingChanged = currentsamCliLocation !== valueAfterInit - currentsamCliLocation = valueAfterInit + if (!args.passive) { + // Force getOrDetectSamCli() to search for SAM CLI. + await samCliConfig.setSamCliLocation('') + } + + const sam = await samCliConfig.getOrDetectSamCli() + const notFound = sam.path === '' - if (args.showMessage !== false) { - if (!valueAfterInit) { - notifyUserSamCliNotDetected(samCliConfig) - } else if (isUserSettingChanged || args.showMessage === true) { - const message = - !isAutoDetected && !isUserSettingChanged - ? getSettingsNotUpdatedMessage(valueBeforeInit ?? '?') - : getSettingsUpdatedMessage(currentsamCliLocation ?? '?') + // Update the user setting. + // + // NOTE: We must NOT _passively_ auto-update the user setting, that + // conflicts with VSCode "remote": each VSCode instance will update the + // setting based on its local environment, but the user settings are + // shared across VSCode instances... + if (!args.passive && sam.autoDetected) { + await samCliConfig.setSamCliLocation(sam.path) + } - vscode.window.showInformationMessage(message) + if (args.showMessage !== false || notFound) { + if (notFound) { + notifyUserSamCliNotDetected(samCliConfig) + } else if (args.showMessage === true) { + vscode.window.showInformationMessage(getSettingsUpdatedMessage(sam.path ?? '?')) } } if (!args.passive) { - recordSamDetect({ result: currentsamCliLocation === undefined ? 'Failed' : 'Succeeded' }) + recordSamDetect({ result: sam.path ? 'Succeeded' : 'Failed' }) } }) } @@ -100,9 +104,3 @@ function getSettingsUpdatedMessage(location: string): string { return `${settingsUpdated} ${configuredLocation}` } - -function getSettingsNotUpdatedMessage(location: string): string { - const configuredLocation = localize('AWS.samcli.configured.location', 'SAM CLI Location: {0}', location) - - return `${settingsNotUpdated} ${configuredLocation}` -} diff --git a/src/shared/sam/cli/samCliInvoker.ts b/src/shared/sam/cli/samCliInvoker.ts index 01535f39198..d9cb2d0546d 100644 --- a/src/shared/sam/cli/samCliInvoker.ts +++ b/src/shared/sam/cli/samCliInvoker.ts @@ -42,24 +42,21 @@ export class DefaultSamCliProcessInvoker implements SamCliProcessInvoker { public async invoke(options?: SamCliProcessInvokeOptions): Promise { const invokeOptions = makeRequiredSamCliProcessInvokeOptions(options) + const sam = await this.context.cliConfig.getOrDetectSamCli() + if (sam.path === '') { + const err = new Error('SAM CLI not found') + getLogger().error(err) + throw err + } else if (sam.autoDetected) { + getLogger().warn('SAM CLI not configured, using SAM found at: %O', sam.path) + } + const childProcess: ChildProcess = new ChildProcess( - this.samCliLocation, + sam.path, invokeOptions.spawnOptions, ...invokeOptions.arguments ) return await childProcess.run() } - - // Gets SAM CLI Location, throws if not found - private get samCliLocation(): string { - const samCliLocation: string | undefined = this.context.cliConfig.getSamCliLocation() - if (!samCliLocation) { - const err = new Error('SAM CLI location not configured') - getLogger().error(err) - throw err - } - - return samCliLocation - } } diff --git a/src/shared/sam/cli/samCliLocalInvoke.ts b/src/shared/sam/cli/samCliLocalInvoke.ts index 4e12170e59f..9da90d38aa6 100644 --- a/src/shared/sam/cli/samCliLocalInvoke.ts +++ b/src/shared/sam/cli/samCliLocalInvoke.ts @@ -209,7 +209,15 @@ export class SamCliLocalInvokeInvocation { public async execute(timeout?: Timeout): Promise { await this.validate() - const samCommand = this.invokerContext.cliConfig.getSamCliLocation() ?? 'sam' + const sam = await this.invokerContext.cliConfig.getOrDetectSamCli() + if (sam.path === '') { + // Don't throw here, let the command fail below. + getLogger().error('SAM CLI not found') + } else if (sam.autoDetected) { + getLogger().warn('SAM CLI not configured, using SAM found at: %O', sam.path) + } + + const samCommand = sam.path ? sam.path : 'sam' const invokeArgs = [ 'local', 'invoke', diff --git a/src/shared/sam/cli/samCliValidator.ts b/src/shared/sam/cli/samCliValidator.ts index 22b6a7f050b..6885e0e7d6c 100644 --- a/src/shared/sam/cli/samCliValidator.ts +++ b/src/shared/sam/cli/samCliValidator.ts @@ -55,7 +55,7 @@ export interface SamCliValidator { } export interface SamCliValidatorContext { - samCliLocation: string | undefined + samCliLocation(): Promise getSamCliExecutableId(): Promise getSamCliInfo(): Promise } @@ -71,10 +71,9 @@ export class DefaultSamCliValidator implements SamCliValidator { samCliFound: false, } - const samCliLocation = this.context.samCliLocation - if (samCliLocation) { + const sam = await this.context.samCliLocation() + if (sam) { result.samCliFound = true - result.versionValidation = await this.getVersionValidatorResult() } @@ -134,18 +133,18 @@ export class DefaultSamCliValidatorContext implements SamCliValidatorContext { private readonly invoker: SamCliProcessInvoker ) {} - public get samCliLocation(): string | undefined { - return this.samCliConfiguration.getSamCliLocation() + public async samCliLocation(): Promise { + return (await this.samCliConfiguration.getOrDetectSamCli()).path } public async getSamCliExecutableId(): Promise { // Function should never get called if there is no SAM CLI - if (!this.samCliLocation) { + if (!(await this.samCliLocation())) { throw new Error('SAM CLI does not exist') } // The modification timestamp of SAM CLI is used as the "distinct executable id" - const stats = await stat(this.samCliLocation) + const stats = await stat(await this.samCliLocation()) return stats.mtime.valueOf().toString() } diff --git a/src/shared/utilities/childProcess.ts b/src/shared/utilities/childProcess.ts index 9af4e1d722d..5822c18d509 100644 --- a/src/shared/utilities/childProcess.ts +++ b/src/shared/utilities/childProcess.ts @@ -72,7 +72,7 @@ export class ChildProcess { throw new Error('process already started') } - getLogger().info(`Running command: ${this.process} ${this.args.join(' ')}`) + getLogger().info(`Running: ${this.process} ${this.args.join(' ')}`) // Async. // See also crossSpawn.spawnSync(). diff --git a/src/test/shared/sam/cli/samCliConfiguration.test.ts b/src/test/shared/sam/cli/samCliConfiguration.test.ts index 140e0341ca3..23eacf83563 100644 --- a/src/test/shared/sam/cli/samCliConfiguration.test.ts +++ b/src/test/shared/sam/cli/samCliConfiguration.test.ts @@ -45,30 +45,6 @@ describe('SamCliConfiguration', () => { assert.strictEqual(samCliConfig.getSamCliLocation(), fakeCliLocation) }) - it('calls location provider when config references file that does not exist', async () => { - let timesCalled: number = 0 - const fakeCliLocation = path.join(tempFolder, 'fakeSamCli') - - await settingsConfiguration.writeSetting( - DefaultSamCliConfiguration.CONFIGURATION_KEY_SAMCLI_LOCATION, - fakeCliLocation, - '' - ) - - const samCliConfig: SamCliConfiguration = new DefaultSamCliConfiguration(settingsConfiguration, { - getLocation: async (): Promise => { - timesCalled++ - - return Promise.resolve(fakeCliLocation) - }, - }) - - await samCliConfig.initialize() - - assert.strictEqual(samCliConfig.getSamCliLocation(), fakeCliLocation) - assert.strictEqual(timesCalled, 1) - }) - it('calls location provider when config not set', async () => { let timesCalled: number = 0 @@ -108,7 +84,7 @@ describe('SamCliConfiguration', () => { await samCliConfig.initialize() - assert.strictEqual(samCliConfig.getSamCliLocation(), undefined) + assert.strictEqual(samCliConfig.getSamCliLocation(), '') }) function createSampleFile(filename: string): void { diff --git a/src/test/shared/sam/cli/samCliValidator.test.ts b/src/test/shared/sam/cli/samCliValidator.test.ts index 36b37b81f4a..5611c284b89 100644 --- a/src/test/shared/sam/cli/samCliValidator.test.ts +++ b/src/test/shared/sam/cli/samCliValidator.test.ts @@ -17,10 +17,14 @@ describe('DefaultSamCliValidator', async () => { class TestSamCliValidatorContext implements SamCliValidatorContext { public samCliVersionId: string = new Date().valueOf().toString() public getInfoCallCount: number = 0 - public samCliLocation: string | undefined + public mockSamLocation: string = '' public constructor(public samCliVersion: string) {} + public async samCliLocation(): Promise { + return this.mockSamLocation + } + public async getSamCliExecutableId(): Promise { return this.samCliVersionId } @@ -65,7 +69,7 @@ describe('DefaultSamCliValidator', async () => { samCliVersionTestScenarios.forEach(test => { it(`handles case where SAM CLI exists and ${test.situation}`, async () => { const validatorContext = new TestSamCliValidatorContext(test.version) - validatorContext.samCliLocation = 'somesamclipath' + validatorContext.mockSamLocation = 'somesamclipath' const samCliValidator = new DefaultSamCliValidator(validatorContext) const validatorResult = await samCliValidator.detectValidSamCli() @@ -84,7 +88,7 @@ describe('DefaultSamCliValidator', async () => { it('handles case where SAM CLI is not found', async () => { const validatorContext = new TestSamCliValidatorContext('') - validatorContext.samCliLocation = undefined + validatorContext.mockSamLocation = '' const samCliValidator = new DefaultSamCliValidator(validatorContext) const validatorResult = await samCliValidator.detectValidSamCli()