Defer root stack creation to first `amplify push` #7174

edwardfoyle · 2021-04-23T22:40:54Z

Description of changes

When running amplify init or amplify env add no root stack is created until the first amplify push. To maintain backwards compatibility with headless scripts, when doing headless init or headless env add, a root stack will be pushed immediately.

Rough implementation details are:
On init / env add (which use the same codepath), it checks if the command has args or not. If not, it does not push the root stack or initialize the amplify app
On push, if the root stack does not exist, it is created. This also involves the "onSuccess" function from amplify-cli being passed into push as a callback because we now have to execute part of the init flow on push and need access to the function after the initial push completes. Passing this in as a callback seemed preferable over trying to orchestrate an "init" call internally from the cli before starting the push, especially since only a subset of the init flow needs to run in the case of a deferred root stack push.

Issue #, if available

This is part of the work to support IAM Role permission boundaries as it gives developers a chance to configure the env before any roles are created. However, it is also a standalone DX improvement that makes init faster and less confusing (new developers won't be confused about why CFN logs are being spit out on init).

Description of how you validated changes

manually tested and added new e2e tests

Checklist

PR description included
yarn test passes
Tests are changed or added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lgtm-com · 2021-04-23T22:56:13Z

This pull request introduces 1 alert when merging d890f86 into ec1a5a7 - view on LGTM.com

new alerts:

1 for Useless conditional

lgtm-com · 2021-04-23T23:20:59Z

This pull request introduces 1 alert when merging 83d81f8 into ec1a5a7 - view on LGTM.com

new alerts:

1 for Useless conditional

codecov-commenter · 2021-04-23T23:22:31Z

Codecov Report

Merging #7174 (c12f99a) into master (fedb6c4) will decrease coverage by 0.01%.
The diff coverage is 65.85%.

@@            Coverage Diff             @@
##           master    #7174      +/-   ##
==========================================
- Coverage   52.25%   52.24%   -0.02%     
==========================================
  Files         495      496       +1     
  Lines       25401    25434      +33     
  Branches     4982     4993      +11     
==========================================
+ Hits        13274    13287      +13     
- Misses      11172    11197      +25     
+ Partials      955      950       -5

Impacted Files	Coverage Δ
packages/amplify-cli-core/src/index.ts	`95.23% <ø> (ø)`
.../src/extensions/amplify-helpers/resource-status.ts	`6.33% <0.00%> (-0.03%)`	⬇️
packages/amplify-cli/src/initialize-env.ts	`10.93% <0.00%> (-0.36%)`	⬇️
...ackages/amplify-cli/src/init-steps/s9-onSuccess.ts	`14.42% <11.11%> (+0.28%)`	⬆️
...rovider-awscloudformation/src/ensure-root-stack.ts	`41.66% <41.66%> (ø)`
...y-provider-awscloudformation/src/push-resources.ts	`10.43% <50.00%> (+0.16%)`	⬆️
...rc/extensions/amplify-helpers/envResourceParams.ts	`84.81% <66.66%> (-0.91%)`	⬇️
...lify-provider-awscloudformation/src/initializer.ts	`53.38% <87.75%> (ø)`
...rc/extensions/amplify-helpers/get-root-stack-id.ts	`88.88% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fedb6c4...c12f99a. Read the comment docs.

packages/amplify-cli/src/commands/push.ts

packages/amplify-cli/src/init-steps/postInitSetup.ts

packages/amplify-cli/src/init-steps/s9-onSuccess.ts

packages/amplify-provider-awscloudformation/src/delete-env.js

packages/amplify-provider-awscloudformation/src/initializer.ts

attilah

Overall looks good, I added a bunch of questions about the implementation and some requests for small changes.

We need to make sure that existing Amplify Console headless workflows will not break with pull into empty dir and such.

attilah · 2021-05-05T20:06:58Z

.circleci/config.yml

@@ -1127,22 +1127,30 @@ jobs:
    environment:


I think you need to add these to split-e2e-tests.ts that will generate this file based on config-base.yml otherwise these will be lost.

this is generated by the split script. The script will automatically add any files in the tests dir

packages/amplify-provider-awscloudformation/src/delete-env.js

attilah · 2021-05-05T20:10:12Z

packages/amplify-provider-awscloudformation/src/__tests__/initializer.test.ts

@@ -33,6 +33,10 @@ describe('run', () => {
      amplify: {
        getTags: jest.fn(),
      },
+      input: {


Why do we need to modify the test to have these arguments?

This test is checking that the pre-push transformer runs during initialization, but now it only runs during a headless init so we need to simulate that in the test

attilah · 2021-05-05T20:11:57Z

packages/amplify-e2e-core/src/init/deleteProject.ts

@@ -2,6 +2,9 @@ import { nspawn as spawn, retry, getCLIPath, describeCloudFormationStack, getPro

 export const deleteProject = async (cwd: string, profileConfig?: any): Promise<void> => {
  const { StackName: stackName, Region: region } = getProjectMeta(cwd).providers.awscloudformation;
+  if (!stackName || !region) {


Was it a missing check causing test failures or is it due to changes in init?

We have some tests that just do an init and then check some local state but then the afterEach step tries to delete the project and fails here because the stack doesn't exist yet

attilah · 2021-05-05T20:13:38Z

packages/amplify-cli/src/extensions/amplify-helpers/get-root-stack-id.ts

-  if (envTeamProviderInfo && envTeamProviderInfo.awscloudformation) {
-    const stackId = envTeamProviderInfo.awscloudformation.StackId;
+  const stackId = envTeamProviderInfo?.awscloudformation?.StackId;
+  if (typeof stackId === 'string') {
    return stackId.split('/')[2];
  }
  throw new Error('Root stack Id not found');


Shall we enhance the error to show what was the value of stackId that we considered "not found"?

attilah · 2021-05-05T20:14:19Z

packages/amplify-cli/src/extensions/amplify-helpers/get-root-stack-id.ts

-  if (envTeamProviderInfo && envTeamProviderInfo.awscloudformation) {
-    const stackId = envTeamProviderInfo.awscloudformation.StackId;
+  const stackId = envTeamProviderInfo?.awscloudformation?.StackId;
+  if (typeof stackId === 'string') {


What else could it be if not undefined? why checking for type here?

It should only ever be a string or undefined but checking for string is a stronger assertion than checking for not undefined

attilah · 2021-05-05T20:17:05Z

packages/amplify-cli/src/extensions/amplify-helpers/envResourceParams.ts

@@ -75,7 +75,12 @@ export function saveEnvResourceParameters(context: $TSContext, category: string,
    stateManager.setTeamProviderInfo(undefined, teamProviderInfo);
    // write hostedUIProviderCreds to deploymentSecrets
    const deploymentSecrets = stateManager.getDeploymentSecrets();
-    const rootStackId = getRootStackId();
+    let rootStackId;
+    try {


Based on usage of getRootStackId we could do better, other than throwing, then swallowing. Other callers are doing something similar which could be enhanced inside that function.

attilah · 2021-05-05T20:21:02Z

packages/amplify-cli/src/init-steps/s9-onSuccess.ts

+  stateManager.setMeta(projectPath, meta);
+
+  // store current cloud meta
+  const currMeta = isNewEnv ? {} : stateManager.getCurrentMeta(projectPath, { throwIfNotExist: false }) || {};


nit: currentMeta

The previous code was setting the same meta object into both files, why do we need to change the logic to read in the file, merge, and store it individually?

previously, this code was only called during env initialization when there was nothing preexisting in the meta files. But now this could be called after adding resources to the env so we need to make sure we don't erase what's there

attilah · 2021-05-05T20:27:44Z

packages/amplify-provider-awscloudformation/src/push-resources.ts

@@ -66,6 +67,7 @@ const deploymentInProgressErrorMessage = (context: $TSContext) => {
 };

 export async function run(context: $TSContext, resourceDefinition: $TSObject) {
+  context = await ensureRootStack(context);


We should solve this differently and not mutate the incoming parameter here, ideally that should have a 'readonly' modifier

The problem is the existing stack initialization code mutates context so therefore the ensureRootStack function also does (because it calls into the existing code if not initialized). ensureRootStack doesn't need to return context and it doesn't need to be reassigned here but I did it that way to make it more obvious that this function is mutating the context

lgtm-com · 2021-05-06T19:47:03Z

This pull request introduces 1 alert when merging 6792dda into 7aed706 - view on LGTM.com

new alerts:

1 for Useless conditional

lgtm-com · 2021-05-06T23:44:50Z

This pull request introduces 1 alert when merging c12f99a into cda63df - view on LGTM.com

new alerts:

1 for Useless conditional

This reverts commit d28dd1c.

…" (#7306) This reverts commit d28dd1c.

github-actions · 2021-05-14T02:42:05Z

👋 Hi, this pull request was referenced in the v4.51.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.51.0.

…7174)

…amplify#7174)" (aws-amplify#7306) This reverts commit d28dd1c.

edwardfoyle added 5 commits April 22, 2021 15:33

feat: defer root stack push to first amplify push

1c23274

fix: reset ccb dir on non-pushed env checkout

18fd3f0

fix: skip cloud deletion on non-pushed envs

fe5904a

test: adding defer init e2e tests

814b1ae

test: fixing e2e tests

d890f86

edwardfoyle requested a review from a team as a code owner April 23, 2021 22:40

edwardfoyle changed the title ~~Defer init~~ Defer root stack creation to first amplify push Apr 23, 2021

test: fix unit test stub

83d81f8

jhockett reviewed Apr 28, 2021

View reviewed changes

edwardfoyle assigned attilah May 3, 2021

attilah approved these changes May 5, 2021

View reviewed changes

edwardfoyle added 2 commits May 6, 2021 12:15

chore: address PR comments

c60fe33

Merge remote-tracking branch 'upstream/master' into defer-init

6792dda

fix: ts build issue

c12f99a

edwardfoyle merged commit d28dd1c into aws-amplify:master May 10, 2021

edwardfoyle added a commit that referenced this pull request May 11, 2021

Revert "feat: defer root stack creation to first amplify push (#7174)"

c40f42a

This reverts commit d28dd1c.

edwardfoyle mentioned this pull request May 11, 2021

Revert "Defer root stack creation to first amplify push" #7306

Merged

ammarkarachi pushed a commit that referenced this pull request May 11, 2021

Revert "feat: defer root stack creation to first amplify push (#7174)…

78854eb

…" (#7306) This reverts commit d28dd1c.

github-actions bot added the referenced-in-release Issues referenced in a published release changelog label May 14, 2021

cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021

feat: defer root stack creation to first amplify push (aws-amplify#…

872602e

…7174)

cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021

Revert "feat: defer root stack creation to first amplify push (aws-…

28b6a21

…amplify#7174)" (aws-amplify#7306) This reverts commit d28dd1c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defer root stack creation to first `amplify push` #7174

Defer root stack creation to first `amplify push` #7174

edwardfoyle commented Apr 23, 2021

lgtm-com bot commented Apr 23, 2021

lgtm-com bot commented Apr 23, 2021

codecov-commenter commented Apr 23, 2021 •

edited

Loading

attilah left a comment

attilah May 5, 2021

edwardfoyle May 5, 2021

attilah May 5, 2021

edwardfoyle May 6, 2021

attilah May 5, 2021

edwardfoyle May 6, 2021

attilah May 5, 2021

attilah May 5, 2021

edwardfoyle May 6, 2021

attilah May 5, 2021

attilah May 5, 2021

edwardfoyle May 6, 2021 •

edited

Loading

attilah May 5, 2021

edwardfoyle May 6, 2021

lgtm-com bot commented May 6, 2021

lgtm-com bot commented May 6, 2021

github-actions bot commented May 14, 2021

Defer root stack creation to first amplify push #7174

Defer root stack creation to first amplify push #7174

Conversation

edwardfoyle commented Apr 23, 2021

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

lgtm-com bot commented Apr 23, 2021

lgtm-com bot commented Apr 23, 2021

codecov-commenter commented Apr 23, 2021 • edited Loading

Codecov Report

attilah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 6, 2021

lgtm-com bot commented May 6, 2021

github-actions bot commented May 14, 2021

Defer root stack creation to first `amplify push` #7174

Defer root stack creation to first `amplify push` #7174

codecov-commenter commented Apr 23, 2021 •

edited

Loading

edwardfoyle May 6, 2021 •

edited

Loading