From d16794437d0e3b65689b07d1bb2263c0fb16772e Mon Sep 17 00:00:00 2001 From: mbfreder Date: Mon, 17 Feb 2025 13:49:53 -0800 Subject: [PATCH 1/4] pass in the negative version of --use-container when using build quickpick --- packages/core/src/shared/sam/build.ts | 9 ++++++++- packages/core/src/test/shared/sam/build.test.ts | 9 +++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/core/src/shared/sam/build.ts b/packages/core/src/shared/sam/build.ts index 233c6f6d7ae..0f642aedf7f 100644 --- a/packages/core/src/shared/sam/build.ts +++ b/packages/core/src/shared/sam/build.ts @@ -29,6 +29,7 @@ import { } from './utils' import { getConfigFileUri, validateSamBuildConfig } from './config' import { runInTerminal } from './processTerminal' +import { getLogger } from '../logger' const buildMementoRootKey = 'samcli.build.params' export interface BuildParams { @@ -213,7 +214,7 @@ export async function runBuild(arg?: TreeNode): Promise { // refactor const buildFlags: string[] = params.paramsSource === ParamsSource.Specify && params.buildFlags - ? JSON.parse(params.buildFlags) + ? resolveBuildFlags(JSON.parse(params.buildFlags)) : await getBuildFlags(params.paramsSource, projectRoot, defaultFlags) // todo remove @@ -281,3 +282,9 @@ function resolveBuildArgConflict(boundArgs: string[]): string[] { // } return Array.from(boundArgsSet) } +export function resolveBuildFlags(buildFlags: string[]): string[] { + if (!buildFlags.includes('--use-container')) { + buildFlags.push('--no-use-container') + } + return buildFlags +} diff --git a/packages/core/src/test/shared/sam/build.test.ts b/packages/core/src/test/shared/sam/build.test.ts index 4375f792987..e374cf55525 100644 --- a/packages/core/src/test/shared/sam/build.test.ts +++ b/packages/core/src/test/shared/sam/build.test.ts @@ -16,6 +16,7 @@ import { createParamsSourcePrompter, getBuildFlags, ParamsSource, + resolveBuildFlags, runBuild, } from '../../../shared/sam/build' import { TreeNode } from '../../../shared/treeview/resourceTreeDataProvider' @@ -229,6 +230,14 @@ describe('SAM build helper functions', () => { assert.deepStrictEqual(quickPick.items, expectedItems) }) }) + + describe('resolveBuildFlags', () => { + it('should add --no-use-container if the buildFlags array does not contain --use-container', () => { + const buildFlags: string[] = ['--cached', '--debug', '--parallel'] + const resolvedBuildFlags = ['--cached', '--debug', '--parallel', '--no-use-container'] + assert.deepStrictEqual(resolvedBuildFlags, resolveBuildFlags(buildFlags)) + }) + }) }) describe('SAM runBuild', () => { From ccb4177d5cf13430bd2a7e27a257746c9aac0318 Mon Sep 17 00:00:00 2001 From: mbfreder Date: Mon, 17 Feb 2025 14:16:54 -0800 Subject: [PATCH 2/4] add changelog --- .../Bug Fix-f7a3ecad-06a7-48cd-896b-2285ea95bd84.json | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 packages/toolkit/.changes/next-release/Bug Fix-f7a3ecad-06a7-48cd-896b-2285ea95bd84.json diff --git a/packages/toolkit/.changes/next-release/Bug Fix-f7a3ecad-06a7-48cd-896b-2285ea95bd84.json b/packages/toolkit/.changes/next-release/Bug Fix-f7a3ecad-06a7-48cd-896b-2285ea95bd84.json new file mode 100644 index 00000000000..80e69eae92a --- /dev/null +++ b/packages/toolkit/.changes/next-release/Bug Fix-f7a3ecad-06a7-48cd-896b-2285ea95bd84.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "appBuilder: pass '--no-use-container' when '--use-container' is not selected in quickpick" +} From cc8f08d48c080eba8af744575a5da1856192a22b Mon Sep 17 00:00:00 2001 From: mbfreder Date: Mon, 17 Feb 2025 16:29:20 -0800 Subject: [PATCH 3/4] address comments --- packages/core/src/shared/sam/build.ts | 15 +++++--- .../core/src/test/shared/sam/build.test.ts | 38 ++++++++++++++++++- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/packages/core/src/shared/sam/build.ts b/packages/core/src/shared/sam/build.ts index 0f642aedf7f..ee03fd07032 100644 --- a/packages/core/src/shared/sam/build.ts +++ b/packages/core/src/shared/sam/build.ts @@ -29,7 +29,7 @@ import { } from './utils' import { getConfigFileUri, validateSamBuildConfig } from './config' import { runInTerminal } from './processTerminal' -import { getLogger } from '../logger' +import { SemVer } from 'semver' const buildMementoRootKey = 'samcli.build.params' export interface BuildParams { @@ -211,10 +211,13 @@ export async function runBuild(arg?: TreeNode): Promise { const projectRoot = params.projectRoot const defaultFlags: string[] = ['--cached', '--parallel', '--save-params', '--use-container'] + + const { path: samCliPath, parsedVersion } = await getSamCliPathAndVersion() + // refactor const buildFlags: string[] = params.paramsSource === ParamsSource.Specify && params.buildFlags - ? resolveBuildFlags(JSON.parse(params.buildFlags)) + ? await resolveBuildFlags(JSON.parse(params.buildFlags), parsedVersion) : await getBuildFlags(params.paramsSource, projectRoot, defaultFlags) // todo remove @@ -230,8 +233,6 @@ export async function runBuild(arg?: TreeNode): Promise { await updateRecentResponse(buildMementoRootKey, 'global', 'templatePath', templatePath) try { - const { path: samCliPath } = await getSamCliPathAndVersion() - // Create a child process to run the SAM build command const buildProcess = new ChildProcess(samCliPath, ['build', ...buildFlags], { spawnOptions: await addTelemetryEnvVar({ @@ -282,7 +283,11 @@ function resolveBuildArgConflict(boundArgs: string[]): string[] { // } return Array.from(boundArgsSet) } -export function resolveBuildFlags(buildFlags: string[]): string[] { +export async function resolveBuildFlags(buildFlags: string[], samCliVersion: SemVer | null): Promise { + // const samCliVersion = (await getSamCliPathAndVersion()).parsedVersion + if (samCliVersion?.compare('1.133.0') ?? -1 < 0) { + return buildFlags + } if (!buildFlags.includes('--use-container')) { buildFlags.push('--no-use-container') } diff --git a/packages/core/src/test/shared/sam/build.test.ts b/packages/core/src/test/shared/sam/build.test.ts index e374cf55525..0d2ae170155 100644 --- a/packages/core/src/test/shared/sam/build.test.ts +++ b/packages/core/src/test/shared/sam/build.test.ts @@ -27,12 +27,14 @@ import { getProjectRootUri } from '../../../shared/sam/utils' import sinon from 'sinon' import { createMultiPick, DataQuickPickItem } from '../../../shared/ui/pickerPrompter' import * as config from '../../../shared/sam/config' +import * as utils from '../../../shared/sam/utils' import { PrompterTester } from '../wizards/prompterTester' import { getWorkspaceFolder, TestFolder } from '../../testUtil' import { samconfigCompleteData, validTemplateData } from './samTestUtils' import { CloudFormationTemplateRegistry } from '../../../shared/fs/templateRegistry' import { getTestWindow } from '../vscode/window' import { CancellationError } from '../../../shared/utilities/timeoutUtils' +import { SemVer } from 'semver' describe('SAM BuildWizard', async function () { const createTester = async (params?: Partial, arg?: TreeNode | undefined) => @@ -232,10 +234,42 @@ describe('SAM build helper functions', () => { }) describe('resolveBuildFlags', () => { - it('should add --no-use-container if the buildFlags array does not contain --use-container', () => { + let sandbox: sinon.SinonSandbox + let pathAndVersionStub: sinon.SinonStub + + beforeEach(() => { + sandbox = sinon.createSandbox() + }) + + afterEach(() => { + sandbox.restore() + }) + + it('should add --no-use-container if the buildFlags array does not contain --use-container', async () => { + const normalVersion = { path: 'file:///path/to/cli', parsedVersion: new SemVer('1.133.0') } + pathAndVersionStub = sandbox.stub().resolves(normalVersion) + sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) const buildFlags: string[] = ['--cached', '--debug', '--parallel'] const resolvedBuildFlags = ['--cached', '--debug', '--parallel', '--no-use-container'] - assert.deepStrictEqual(resolvedBuildFlags, resolveBuildFlags(buildFlags)) + assert.deepEqual(resolvedBuildFlags, await resolveBuildFlags(buildFlags, normalVersion.parsedVersion)) + }) + + it('should return the original buildFlags array if SAM CLI version is less than 1.133', async () => { + const lowerVersion = { path: 'file:///path/to/cli', parsedVersion: new SemVer('1.110.0') } + const pathAndVersionStub = sandbox.stub().resolves(lowerVersion) + sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) + const buildFlags = ['--cached', '--parallel', '--save-params'] + const resolvedBuildFlags = await resolveBuildFlags(buildFlags, lowerVersion.parsedVersion) + assert.deepEqual(resolvedBuildFlags, buildFlags) + }) + + it('should not add "--no-use-container" if "--use-container" is already present', async () => { + const normalVersion = { path: 'file:///path/to/cli', parsedVersion: new SemVer('1.110.0') } + pathAndVersionStub = sandbox.stub().resolves(normalVersion) + sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) + const buildFlags = ['--cached', '--parallel', '--save-params', '--use-container'] + const resolvedBuildFlags = await resolveBuildFlags(buildFlags, normalVersion.parsedVersion) + assert.deepStrictEqual(resolvedBuildFlags, buildFlags) }) }) }) From 1caee50a0f70fdeb1c6fe3ec0712d2417b781d49 Mon Sep 17 00:00:00 2001 From: mbfreder Date: Tue, 18 Feb 2025 10:16:31 -0800 Subject: [PATCH 4/4] refactor tests --- packages/core/src/shared/sam/build.ts | 2 +- .../core/src/test/shared/sam/build.test.ts | 47 ++++++++++--------- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/packages/core/src/shared/sam/build.ts b/packages/core/src/shared/sam/build.ts index ee03fd07032..81179177b83 100644 --- a/packages/core/src/shared/sam/build.ts +++ b/packages/core/src/shared/sam/build.ts @@ -284,7 +284,7 @@ function resolveBuildArgConflict(boundArgs: string[]): string[] { return Array.from(boundArgsSet) } export async function resolveBuildFlags(buildFlags: string[], samCliVersion: SemVer | null): Promise { - // const samCliVersion = (await getSamCliPathAndVersion()).parsedVersion + // --no-use-container was not added until v1.133.0 if (samCliVersion?.compare('1.133.0') ?? -1 < 0) { return buildFlags } diff --git a/packages/core/src/test/shared/sam/build.test.ts b/packages/core/src/test/shared/sam/build.test.ts index 0d2ae170155..a7663788205 100644 --- a/packages/core/src/test/shared/sam/build.test.ts +++ b/packages/core/src/test/shared/sam/build.test.ts @@ -235,8 +235,6 @@ describe('SAM build helper functions', () => { describe('resolveBuildFlags', () => { let sandbox: sinon.SinonSandbox - let pathAndVersionStub: sinon.SinonStub - beforeEach(() => { sandbox = sinon.createSandbox() }) @@ -245,31 +243,25 @@ describe('SAM build helper functions', () => { sandbox.restore() }) - it('should add --no-use-container if the buildFlags array does not contain --use-container', async () => { - const normalVersion = { path: 'file:///path/to/cli', parsedVersion: new SemVer('1.133.0') } - pathAndVersionStub = sandbox.stub().resolves(normalVersion) - sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) - const buildFlags: string[] = ['--cached', '--debug', '--parallel'] - const resolvedBuildFlags = ['--cached', '--debug', '--parallel', '--no-use-container'] - assert.deepEqual(resolvedBuildFlags, await resolveBuildFlags(buildFlags, normalVersion.parsedVersion)) + it('uses --no-use-container when --use-container is absent', async () => { + const normalVersion = new SemVer('1.133.0') + const buildFlags = ['--cached', '--debug', '--parallel'] + const expectedBuildFlags = ['--cached', '--debug', '--parallel', '--no-use-container'] + return testResolveBuildFlags(sandbox, normalVersion, buildFlags, expectedBuildFlags) }) - it('should return the original buildFlags array if SAM CLI version is less than 1.133', async () => { - const lowerVersion = { path: 'file:///path/to/cli', parsedVersion: new SemVer('1.110.0') } - const pathAndVersionStub = sandbox.stub().resolves(lowerVersion) - sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) + it('preserves buildFlags when SAM CLI version < 1.133', async () => { + const lowerVersion = new SemVer('1.110.0') const buildFlags = ['--cached', '--parallel', '--save-params'] - const resolvedBuildFlags = await resolveBuildFlags(buildFlags, lowerVersion.parsedVersion) - assert.deepEqual(resolvedBuildFlags, buildFlags) + const expectedBuildFlags = ['--cached', '--parallel', '--save-params'] + return testResolveBuildFlags(sandbox, lowerVersion, buildFlags, expectedBuildFlags) }) - it('should not add "--no-use-container" if "--use-container" is already present', async () => { - const normalVersion = { path: 'file:///path/to/cli', parsedVersion: new SemVer('1.110.0') } - pathAndVersionStub = sandbox.stub().resolves(normalVersion) - sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) + it('respects existing --use-container flag', async () => { + const normalVersion = new SemVer('1.110.0') const buildFlags = ['--cached', '--parallel', '--save-params', '--use-container'] - const resolvedBuildFlags = await resolveBuildFlags(buildFlags, normalVersion.parsedVersion) - assert.deepStrictEqual(resolvedBuildFlags, buildFlags) + const expectedBuildFlags = ['--cached', '--parallel', '--save-params', '--use-container'] + return testResolveBuildFlags(sandbox, normalVersion, buildFlags, expectedBuildFlags) }) }) }) @@ -594,3 +586,16 @@ describe('SAM runBuild', () => { }) }) }) + +function testResolveBuildFlags( + sandbox: sinon.SinonSandbox, + parsedVersion: SemVer, + buildFlags: string[], + expectedBuildFlags: string[] +) { + const pathAndVersionStub = sandbox.stub().resolves({ path: 'file:///path/to/cli', parsedVersion }) + sandbox.stub(utils, 'getSamCliPathAndVersion').callsFake(pathAndVersionStub) + return resolveBuildFlags(buildFlags, parsedVersion).then((resolvedBuildFlags) => { + assert.deepEqual(resolvedBuildFlags, expectedBuildFlags) + }) +}