Skip to content
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

fix: clearing the .amplify/generated/env/ before synthesis #2031

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

vigy02
Copy link

@vigy02 vigy02 commented Sep 19, 2024

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.

Changes

  • Added a method in backend_factory.ts to clear the .amplify/generated/env/ directory at the start of each deployment.
  • Updated existing tests to verify that the directory is cleared during deployments.

Validation

  • Reran E2E tests with the updated code to ensure no issues arise from clearing the directory.
  • Manually verified that the directory is indeed cleared before and after deployments.

Checklist

  • Automated test coverage for the change has been added or updated.
  • E2E tests have been run with the run-e2e label set.

@vigy02 vigy02 requested a review from a team as a code owner September 19, 2024 18:33
Copy link

changeset-bot bot commented Sep 19, 2024

🦋 Changeset detected

Latest commit: 2a2b188

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@aws-amplify/backend Patch

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

Comment on lines +102 to +105
const pathToDelete = `${process.cwd()}/.amplify/generated/env/`;

// Clear any existing files and subdirectories in the directory
fs.rmSync(pathToDelete, { recursive: true, force: true });
Copy link
Member

Choose a reason for hiding this comment

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

Two things.

  1. The path should be concatenated with path.join api . This means we also have this problem here - could you include fix for this part in the PR ?
  2. This is a function specific code. The logic should somehow go to backend-function. Possible solutions for that:
    1. Have this as static initializer somewhere, perhaps FunctionEnvironmentTypeGenerator .
    2. Expose lifycycle hooks from ConstructFactory. For example ConstructFactory.beforeAll (having in mind that we might add ConstructFactory.beforeEach some time in the future).

@rtpascual @Amplifiyer wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was initially thinking 2i but we would have to be careful this won't delete the generated directory during synth of multiple functions right?

I'm wondering if we will ever see more use cases similar to this in other packages to warrant 2ii.

Copy link
Member

@sobolk sobolk Sep 19, 2024

Choose a reason for hiding this comment

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

won't delete the generated directory during synth of multiple functions right?

If we make it run once in static context, before any FunctionEnvironmentTypeGenerator ctor runs then in theory this should work. But won't be super testable.

2ii seems like a proper way to hook this up into framework and control the control flow explicitly.
Would we need this else where, hard to say, but we could ride on 2i until second candidate appears.

Copy link
Contributor

@rtpascual rtpascual Sep 19, 2024

Choose a reason for hiding this comment

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

If we make it run once in static context, before any FunctionEnvironmentTypeGenerator ctor runs then in theory this should work. But won't be super testable.

Oh I see what you mean now. Yeah I can only think a good way to test this is in e2e, for example we create app with function -> run sandbox-> update function name -> check for old generated file after hotswap.

I agree it may be too much right now to do 2ii, 2i will work for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants