-
Notifications
You must be signed in to change notification settings - Fork 736
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.
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
Conversation
|
|
One test is failing here but not on my local machine. Investigating this. |
| const { paramsSource, stackName, region, projectRoot } = args | ||
| const shouldWriteSyncSamconfigGlobal = paramsSource !== ParamsSource.SamConfig && !!stackName && !!region | ||
| if (boundArgs.includes('--watch')) { | ||
| shouldWriteSyncSamconfigGlobal && (await writeSamconfigGlobal(projectRoot, stackName, region)) |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
usingstack_nameandregionfromsamconfig.tomlfile. In current
logic, thestack_nameandregionare 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.
| prompterTester.assertCallAll() | ||
| }) | ||
|
|
||
| it('[entry: command palette] specify and save flag (with --watch) should save params before starting SAM process', async () => { |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "type": "Bug Fix", | |||
| "description": "appBuilder refresh feature doesnt work during sync --watch" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this description meaningful to customers?
Problem
When --watch flag is used, the sync process remains alive. The region and stack_name info get written to config file only after the process finishes. This means customer would not be able to see (or refresh) the latest deployed resoures during the sync process.
Solution
Write to samconfig.toml before running sam sync
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.