fix: added static context to clear the .amplify/generated/env Directory before synthesis#2044
Open
vigy02 wants to merge 39 commits intoaws-amplify:mainfrom
Open
fix: added static context to clear the .amplify/generated/env Directory before synthesis#2044vigy02 wants to merge 39 commits intoaws-amplify:mainfrom
vigy02 wants to merge 39 commits intoaws-amplify:mainfrom
Conversation
…ry before synthesis
…ry before synthesis
…ry before synthesis
🦋 Changeset detectedLatest commit: cfd19f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
sobolk
reviewed
Sep 24, 2024
Contributor
sobolk
left a comment
sobolk
left a comment
There was a problem hiding this comment.
LGTM. I like the approach.
Couple of naming comments.
| * Generates a typed process.env shim for environment variables | ||
| */ | ||
| export class FunctionEnvironmentTypeGenerator { | ||
| // Using this to ensure directory is cleared once |
Contributor
There was a problem hiding this comment.
Suggested change
| // Using this to ensure directory is cleared once | |
| // Using this to ensure directory is cleared once per synthesis |
| /** | ||
| * Clear existing files and subdirectories in the generated env directory | ||
| */ | ||
| public clearGeneratedEnvDirectory = (): void => { |
Contributor
Author
There was a problem hiding this comment.
Can't be private since we're using this in test class
Contributor
There was a problem hiding this comment.
If we're calling this in ctor.
This test could be changed to just use multiple ctor calls instead and hide it.
| fsExistsSyncMock.mock.restore(); | ||
| fsRmSyncMock.mock.restore(); | ||
| }); | ||
| const resetFactoryCount = () => { |
Contributor
There was a problem hiding this comment.
Suggested change
| const resetFactoryCount = () => { | |
| const resetFactoryGlobalState = () => { |
sobolk
reviewed
Sep 24, 2024
| /** | ||
| * Clear existing files and subdirectories in the generated env directory | ||
| */ | ||
| public clearGeneratedEnvDirectory = (): void => { |
Contributor
There was a problem hiding this comment.
If we're calling this in ctor.
This test could be changed to just use multiple ctor calls instead and hide it.
Comment on lines
83
to
93
| assert.equal(fsExistsSyncMock.mock.calls.length, 1); | ||
| assert.equal(fsRmSyncMock.mock.calls.length, 1); | ||
|
|
||
| const functionEnvironmentTypeGenerator2 = | ||
| new FunctionEnvironmentTypeGenerator('testFunction2'); | ||
|
|
||
| functionEnvironmentTypeGenerator.clearGeneratedEnvDirectory(); | ||
| functionEnvironmentTypeGenerator2.clearGeneratedEnvDirectory(); | ||
|
|
||
| assert.equal(fsExistsSyncMock.mock.calls.length, 1); | ||
| assert.equal(fsRmSyncMock.mock.calls.length, 1); |
Contributor
There was a problem hiding this comment.
Suggested change
| assert.equal(fsExistsSyncMock.mock.calls.length, 1); | |
| assert.equal(fsRmSyncMock.mock.calls.length, 1); | |
| const functionEnvironmentTypeGenerator2 = | |
| new FunctionEnvironmentTypeGenerator('testFunction2'); | |
| functionEnvironmentTypeGenerator.clearGeneratedEnvDirectory(); | |
| functionEnvironmentTypeGenerator2.clearGeneratedEnvDirectory(); | |
| assert.equal(fsExistsSyncMock.mock.calls.length, 1); | |
| assert.equal(fsRmSyncMock.mock.calls.length, 1); | |
| assert.equal(fsExistsSyncMock.mock.calls.length, 1); | |
| assert.equal(fsRmSyncMock.mock.calls.length, 1); | |
| const functionEnvironmentTypeGenerator2 = | |
| new FunctionEnvironmentTypeGenerator('testFunction2'); | |
| assert.equal(fsExistsSyncMock.mock.calls.length, 1); | |
| assert.equal(fsRmSyncMock.mock.calls.length, 1); |
calling ctor multiple times should be good enough.
sobolk
previously approved these changes
Sep 24, 2024
sobolk
reviewed
Sep 24, 2024
.changeset/rare-seas-eat.md
Outdated
Comment on lines
1
to
3
| --- | ||
| '@aws-amplify/backend-function': patch | ||
| --- |
Contributor
There was a problem hiding this comment.
Suggested change
| --- | |
| '@aws-amplify/backend-function': patch | |
| --- | |
| --- | |
| '@aws-amplify/backend': patch | |
| '@aws-amplify/backend-function': patch | |
| --- |
Looks like new check is working (#2042)
awsluja
previously approved these changes
Sep 24, 2024
vigy02
added
run-e2e
sobolk
approved these changes
Sep 25, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
.amplify/generated/env/directory is not cleared before deployments, causing outdated/renamed environment files to remain. This can lead to confusion and potential errors when the function's handler code does not match the current environment settings.