feat: replace env variable with gen2 branch name #14327
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...mplify-cli/src/commands/gen2-migration/codegen-generate/src/codegen-head/command-handlers.ts
Fixed
Show fixed
Hide fixed
...amplify-cli/src/commands/gen2-migration/codegen-custom-resources/custom-resource-migrator.ts
Fixed
Show fixed
Hide fixed
sanjanaravikumar-az
force-pushed
the
sanjrkmr/aws-branch-env-variable-replacement
branch
5 times, most recently
from
aafa1b2 to
75ea0f5
Compare
kaizencc
left a comment
kaizencc
left a comment
There was a problem hiding this comment.
there's a few things i would like to see before i can give this a proper review:
- the PR description is missing a "why" or a link to an issue where the reviewer can gain context on the motivation behind the PR.
- the PR description "changes" section (or, alternatively, you could review your own PR and add relevant inline comments) does not include deletion of
code-transformers.tsandast-code-transformer.tsand why that is necessary yarn.lockfile should not change if there was no update to dependencies in this PR
the main thing is providing necessary context for a smooth review though. currently any reviewer of your PR needs to have context supplied elsewhere and I just don't have that context yet
| undefined, | ||
| undefined, | ||
| factory.createIdentifier('process.env.AMPLIFY_GEN_1_ENV_NAME ?? "sandbox"'), | ||
| factory.createIdentifier('process.env.AWS_BRANCH ?? "sandbox"'), |
There was a problem hiding this comment.
I had to check the docs to understand this, because on the surface, it just looks like a rename. You mention this change in your PR description, but it's always helpful to also indicate why, and provide a reference.
There was a problem hiding this comment.
noted, i'll add this in the PR description so its more clear.
iankhou
left a comment
iankhou
left a comment
There was a problem hiding this comment.
Remove yarn.lock file changes, provide a clearer explanation in the PR description. Right now it tells me "what," but I want to know "why."
There was a problem hiding this comment.
Don't modify this unless you're modifying dependencies in package.json.
There was a problem hiding this comment.
got it, it wasn't supposed to be there originally, i've deleted it now.
sanjanaravikumar-az
changed the base branch from
gen2-migration
to
sanjrkmr/aws-branch-env-variable-clean
sanjanaravikumar-az
changed the base branch from
sanjrkmr/aws-branch-env-variable-clean
to
sanjrkmr/custom-resource-cdk-stack-parser-and-transformer
sanjanaravikumar-az
changed the base branch from
sanjrkmr/custom-resource-cdk-stack-parser-and-transformer
to
gen2-migration
There was a problem hiding this comment.
this file is unnecessary because i've added basically the same functionality in amplify-helper-transformer.ts, and it is more efficient.
There was a problem hiding this comment.
this is removed for pretty much the same reason as packages/amplify-cli/src/tests/commands/gen2-migration/codegen-custom-resources/transformer/ast-code-transformer.test.ts. its duplicate code that isnt needed anymore.
- Add missing custom-resource-migrator.ts file - Fix DataDefinitionFetcher constructor call to use correct number of arguments
…cess.env.AMPLIFY_ENV
The test was expecting the wrong replacement value. The code transformer
correctly replaces cdk.Fn.ref('env') with branchName, not process.env.AMPLIFY_ENV.
- Add amplify-helper-transformer.ts with branchName transformation
- Transform cdk.Fn.ref('env') and Fn.ref('env') to branchName
- Transform AmplifyHelpers.getProjectInfo().envName to branchName
- Add branchName declaration using process.env.AWS_BRANCH ?? 'sandbox'
- Remove AmplifyHelpers.getProjectInfo() replacement from command-handlers
- Remove in-place Gen1 file transformation to preserve original files
sanjanaravikumar-az
force-pushed
the
sanjrkmr/aws-branch-env-variable-replacement
branch
from
96ad63c to
019426e
Compare
|
@kaizencc I've tried to add a bit more context for the reason behind this change, and I've updated the project board as well as per your suggestion, so hopefully i was able to provide a little more background. I've also added inline comments for the reasoning behind deleting the files I removed, and updated the yarn.lock file so there are no changes to it.
|
3 participants
Initial problem: Previously the migrated code contained AMPLIFY_GEN_1_ENV_NAME = process.env.AMPLIFY_ENV which should be changed to branchName = process.env.AWS_BRANCH. Also any env references would be cdk.Fn.ref('env') which does not suit the gen2 format.
There are patterns like cdk.Fn.ref('env') or AmplifyHelpers.getProjectInfo().envName which need to be transformed to branchName, and we also need to remove the import related to this. Also before, when we migrate the gen1 app to gen2, you get this line of code at the top which is like AMPLIFY_GEN_1_ENV_NAME = process.env.AMPLIFY_ENV which we replace with branchName = process.env.AWS_BRANCH.
The code scans the gen1 app for env variables and references and replaces them with the proper gen2 branch names and references. This is because gen2 apps only use branch names and have no concept of environments.
Changes:
AmplifyHelperTransformerto handle standalonecdk.Fn.ref('env')callsCodeTransformerto replacecdk.Fn.ref('env')withbranchNamevariableconst branchName = process.env.AWS_BRANCH ?? "sandbox". The AWS_BRANCH is used to get the branch name of the current build.AMPLIFY_GEN_1_ENV_NAMEfor compatibilityGenerated CDK stack files will now use:
const branchName = process.env.AWS_BRANCH ?? "sandbox";cdk.Fn.ref('env')replaced withbranchNameAmplifyHelpers.getProjectInfo().envNamereplaced withbranchNameBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.