Skip to content

fix(lambda): save params before running sam command when --watch is selected #6089

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.

Sign up for GitHub

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
merged 4 commits into from
Nov 27, 2024
Merged

fix(lambda): save params before running sam command when --watch is selected #6089

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81951b3
save params before running command when --watch is selected
mbfreder Nov 22, 2024
ce3ba82
Merge branch 'aws:master' into sync-watch-config
mbfreder Nov 25, 2024
b917d66
fix failing test
mbfreder Nov 26, 2024
78b70ef
add changelog
mbfreder Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/core/src/shared/sam/sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,9 +354,15 @@ export async function runSamSync(args: SyncParams) {
}),
})

await runInTerminal(sam, 'sync')
// with '--watch' selected, the sync process will run in the background until the user manually kills it
// we need to save the stack and region to the samconfig file now, otherwise the user would not see latest deployed resoure during this sync process
const { paramsSource, stackName, region, projectRoot } = args
const shouldWriteSyncSamconfigGlobal = paramsSource !== ParamsSource.SamConfig && !!stackName && !!region
if (boundArgs.includes('--watch')) {
shouldWriteSyncSamconfigGlobal && (await writeSamconfigGlobal(projectRoot, stackName, region))
Copy link
Contributor

@vicheey vicheey Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Curious]: Shouldn't we just have the following line before await runInTerminal(sam, 'sync')?

shouldWriteSyncSamconfigGlobal && (await writeSamconfigGlobal(projectRoot, stackName, region))

Copy link
Contributor Author

@mbfreder mbfreder Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could, if there's no specific reason why we added that line after running the command.

Copy link
Contributor

@vicheey vicheey Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding past commit here for historical context:

Auto refresh feature trigger extension to fetch for deployed stack node
using stack_name and region from samconfig.toml file. In current
logic, the stack_name and region are written before deployment
process completed to avoid the edge case when deployment failure due to
stack up-to-date. However, this order triggers the extension to fetch
for stack that may not already been created and through an error toast
message to customer stating "Stack does not exist".

Above is the reason we only save stack_name and region info after deploy/sync process completes. For the case of using --watch with sync, we can proceed with current approach.

}

await runInTerminal(sam, 'sync')
shouldWriteSyncSamconfigGlobal && (await writeSamconfigGlobal(projectRoot, stackName, region))
}

Expand Down
87 changes: 87 additions & 0 deletions packages/core/src/test/shared/sam/sync.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,93 @@ describe('SAM runSync', () => {
prompterTester.assertCallAll()
})

it('[entry: command palette] specify and save flag (with --watch) should save params before starting SAM process', async () => {
Copy link
Contributor

@justinmk3 justinmk3 Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

95% of the 100 lines in this test are identical to the test above it. The Copy-Paste Detection CI job warned about this. This is not acceptable in this codebase.

const prompterTester = PrompterTester.init()
.handleQuickPick('Select a SAM/CloudFormation Template', async (quickPick) => {
// Need sometime to wait for the template to search for template file
await quickPick.untilReady()
assert.strictEqual(quickPick.items[0].label, templateFile.fsPath)
quickPick.acceptItem(quickPick.items[0])
})
.handleQuickPick('Specify parameters for deploy', async (picker) => {
// Need time to check samconfig.toml file and generate options
await picker.untilReady()
assert.strictEqual(picker.items[0].label, 'Specify required parameters and save as defaults')
picker.acceptItem(picker.items[0])
})
.handleQuickPick('Select a region', (quickPick) => {
const select = quickPick.items.filter((i) => i.detail === 'us-west-2')[0]
quickPick.acceptItem(select || quickPick.items[0])
})
.handleQuickPick('Select a CloudFormation Stack', async (picker) => {
await picker.untilReady()
assert.strictEqual(picker.items[2].label, 'stack3')
picker.acceptItem(picker.items[2])
})
.handleQuickPick('Select an S3 Bucket', async (picker) => {
await picker.untilReady()
assert.strictEqual(picker.items.length, 3)
assert.strictEqual(picker.items[2].label, 'stack-3-bucket')
picker.acceptItem(picker.items[2])
})
.handleQuickPick('Specify parameters for sync', async (picker) => {
await picker.untilReady()
assert.strictEqual(picker.items.length, 9)
const dependencyLayer = picker.items.filter((item) => item.label === 'Dependency layer')[0]
const useContainer = picker.items.filter((item) => item.label === 'Use container')[0]
const watch = picker.items.filter((item) => item.label === 'Watch')[0]
picker.acceptItems(dependencyLayer, useContainer, watch)
})
.build()

// Invoke sync command from command palette
await runSync('code', undefined)

assert(mockGetSamCliPath.calledOnce)
assert(mockChildProcessClass.calledOnce)
assert.deepEqual(mockChildProcessClass.getCall(0).args, [
'sam-cli-path',
[
'sync',
'--code',
'--template',
`${templateFile.fsPath}`,
'--s3-bucket',
'stack-3-bucket',
'--stack-name',
'stack3',
'--region',
'us-west-2',
'--no-dependency-layer',
'--save-params',
'--dependency-layer',
'--use-container',
'--watch',
],
{
spawnOptions: {
cwd: projectRoot?.fsPath,
env: {
AWS_TOOLING_USER_AGENT: 'AWS-Toolkit-For-VSCode/testPluginVersion',
SAM_CLI_TELEMETRY: '0',
},
},
},
])
assert(mockGetSpawnEnv.calledOnce)
assert(spyRunInterminal.calledOnce)
assert.deepEqual(spyRunInterminal.getCall(0).args, [mockSamSyncChildProcess, 'sync'])
assert.strictEqual(spyWriteSamconfigGlobal.callCount, 2)
assert(spyWriteSamconfigGlobal.calledBefore(spyRunInterminal))
// Check telementry
assertTelemetry('sam_sync', { result: 'Succeeded', source: undefined })
assertTelemetryCurried('sam_sync')({
syncedResources: 'CodeOnly',
source: undefined,
})
prompterTester.assertCallAll()
})

it('[entry: template file] specify flag should instantiate correct process in terminal', async () => {
const prompterTester = PrompterTester.init()
.handleQuickPick('Specify parameter source for sync', async (picker) => {
Expand Down
Loading