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

Defer root stack creation to first amplify push #7174

Merged
merged 9 commits into from
May 10, 2021
58 changes: 44 additions & 14 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1127,22 +1127,30 @@ jobs:
environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

TEST_SUITE: src/__tests__/function_5.test.ts
CLI_REGION: eu-central-1
defer-init-push-amplify_e2e_tests:
working_directory: ~/repo
docker: *ref_1
resource_class: large
steps: *ref_4
environment:
TEST_SUITE: src/__tests__/defer-init-push.test.ts
CLI_REGION: ap-northeast-1
configure-project-amplify_e2e_tests:
working_directory: ~/repo
docker: *ref_1
resource_class: large
steps: *ref_4
environment:
TEST_SUITE: src/__tests__/configure-project.test.ts
CLI_REGION: ap-northeast-1
CLI_REGION: ap-southeast-1
api_4-amplify_e2e_tests:
working_directory: ~/repo
docker: *ref_1
resource_class: large
steps: *ref_4
environment:
TEST_SUITE: src/__tests__/api_4.test.ts
CLI_REGION: ap-southeast-1
CLI_REGION: ap-southeast-2
schema-iterative-update-4-amplify_e2e_tests_pkg_linux:
working_directory: ~/repo
docker: *ref_1
Expand Down Expand Up @@ -1813,6 +1821,16 @@ jobs:
TEST_SUITE: src/__tests__/function_5.test.ts
CLI_REGION: eu-central-1
steps: *ref_5
defer-init-push-amplify_e2e_tests_pkg_linux:
working_directory: ~/repo
docker: *ref_1
resource_class: large
environment:
AMPLIFY_DIR: /home/circleci/repo/out
AMPLIFY_PATH: /home/circleci/repo/out/amplify-pkg-linux
TEST_SUITE: src/__tests__/defer-init-push.test.ts
CLI_REGION: ap-northeast-1
steps: *ref_5
configure-project-amplify_e2e_tests_pkg_linux:
working_directory: ~/repo
docker: *ref_1
Expand All @@ -1821,7 +1839,7 @@ jobs:
AMPLIFY_DIR: /home/circleci/repo/out
AMPLIFY_PATH: /home/circleci/repo/out/amplify-pkg-linux
TEST_SUITE: src/__tests__/configure-project.test.ts
CLI_REGION: ap-northeast-1
CLI_REGION: ap-southeast-1
steps: *ref_5
api_4-amplify_e2e_tests_pkg_linux:
working_directory: ~/repo
Expand All @@ -1831,7 +1849,7 @@ jobs:
AMPLIFY_DIR: /home/circleci/repo/out
AMPLIFY_PATH: /home/circleci/repo/out/amplify-pkg-linux
TEST_SUITE: src/__tests__/api_4.test.ts
CLI_REGION: ap-southeast-1
CLI_REGION: ap-southeast-2
steps: *ref_5
workflows:
version: 2
Expand Down Expand Up @@ -1948,15 +1966,15 @@ workflows:
- containers-api-amplify_e2e_tests
- interactions-amplify_e2e_tests
- datastore-modelgen-amplify_e2e_tests
- configure-project-amplify_e2e_tests
- defer-init-push-amplify_e2e_tests
- schema-iterative-update-2-amplify_e2e_tests
- schema-data-access-patterns-amplify_e2e_tests
- init-special-case-amplify_e2e_tests
- api_4-amplify_e2e_tests
- auth_1-amplify_e2e_tests
- configure-project-amplify_e2e_tests
- feature-flags-amplify_e2e_tests
- schema-versioned-amplify_e2e_tests
- plugin-amplify_e2e_tests
- api_4-amplify_e2e_tests
- done_with_pkg_linux_e2e_tests:
requires:
- schema-key-amplify_e2e_tests_pkg_linux
Expand All @@ -1978,15 +1996,15 @@ workflows:
- containers-api-amplify_e2e_tests_pkg_linux
- interactions-amplify_e2e_tests_pkg_linux
- datastore-modelgen-amplify_e2e_tests_pkg_linux
- configure-project-amplify_e2e_tests_pkg_linux
- defer-init-push-amplify_e2e_tests_pkg_linux
- schema-iterative-update-2-amplify_e2e_tests_pkg_linux
- schema-data-access-patterns-amplify_e2e_tests_pkg_linux
- init-special-case-amplify_e2e_tests_pkg_linux
- api_4-amplify_e2e_tests_pkg_linux
- auth_1-amplify_e2e_tests_pkg_linux
- configure-project-amplify_e2e_tests_pkg_linux
- feature-flags-amplify_e2e_tests_pkg_linux
- schema-versioned-amplify_e2e_tests_pkg_linux
- plugin-amplify_e2e_tests_pkg_linux
- api_4-amplify_e2e_tests_pkg_linux
- amplify_migration_tests_latest:
context:
- amplify-ecr-image-pull
Expand Down Expand Up @@ -2399,7 +2417,7 @@ workflows:
filters: *ref_9
requires:
- migration-api-key-migration1-amplify_e2e_tests
- configure-project-amplify_e2e_tests:
- defer-init-push-amplify_e2e_tests:
context: *ref_7
post-steps: *ref_8
filters: *ref_9
Expand Down Expand Up @@ -2459,7 +2477,7 @@ workflows:
filters: *ref_9
requires:
- layer-amplify_e2e_tests
- api_4-amplify_e2e_tests:
- configure-project-amplify_e2e_tests:
context: *ref_7
post-steps: *ref_8
filters: *ref_9
Expand Down Expand Up @@ -2519,6 +2537,12 @@ workflows:
filters: *ref_9
requires:
- auth_3-amplify_e2e_tests
- api_4-amplify_e2e_tests:
context: *ref_7
post-steps: *ref_8
filters: *ref_9
requires:
- auth_1-amplify_e2e_tests
- schema-iterative-update-4-amplify_e2e_tests_pkg_linux:
context: &ref_10
- amplify-ecr-image-pull
Expand Down Expand Up @@ -2841,7 +2865,7 @@ workflows:
filters: *ref_12
requires:
- migration-api-key-migration1-amplify_e2e_tests_pkg_linux
- configure-project-amplify_e2e_tests_pkg_linux:
- defer-init-push-amplify_e2e_tests_pkg_linux:
context: *ref_10
post-steps: *ref_11
filters: *ref_12
Expand Down Expand Up @@ -2905,7 +2929,7 @@ workflows:
filters: *ref_12
requires:
- layer-amplify_e2e_tests_pkg_linux
- api_4-amplify_e2e_tests_pkg_linux:
- configure-project-amplify_e2e_tests_pkg_linux:
context: *ref_10
post-steps: *ref_11
filters: *ref_12
Expand Down Expand Up @@ -2969,3 +2993,9 @@ workflows:
filters: *ref_12
requires:
- auth_3-amplify_e2e_tests_pkg_linux
- api_4-amplify_e2e_tests_pkg_linux:
context: *ref_10
post-steps: *ref_11
filters: *ref_12
requires:
- auth_1-amplify_e2e_tests_pkg_linux
3 changes: 2 additions & 1 deletion packages/amplify-cli-core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ interface AmplifyToolkit {
getProjectMeta: () => $TSMeta;
getResourceStatus: (category?: $TSAny, resourceName?: $TSAny, providerName?: $TSAny, filteredResources?: $TSAny) => $TSAny;
getResourceOutputs: () => $TSAny;
getTags: (context: $TSContext) => $TSAny;
getWhen: () => $TSAny;
inputValidation: (input: $TSAny) => $TSAny;
listCategories: () => $TSAny;
Expand All @@ -195,7 +196,7 @@ interface AmplifyToolkit {
filteredResources?: { category: string; resourceName: string }[],
) => $TSAny;
storeCurrentCloudBackend: () => $TSAny;
readJsonFile: () => $TSAny;
readJsonFile: (file: string) => $TSAny;
removeEnvFromCloud: () => $TSAny;
removeDeploymentSecrets: (context: $TSContext, category: string, resource: string) => void;
removeResource: () => $TSAny;
Expand Down
3 changes: 3 additions & 0 deletions packages/amplify-cli/src/commands/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import sequential from 'promise-sequential';
import ora from 'ora';
import { $TSContext, $TSObject, stateManager, exitOnNextTick } from 'amplify-cli-core';
import { getProviderPlugins } from '../extensions/amplify-helpers/get-provider-plugins';
import { onSuccess } from '../init-steps/s9-onSuccess';

const spinner = ora('');

Expand Down Expand Up @@ -44,9 +45,11 @@ export const run = async (context: $TSContext) => {
if (context.parameters.options.force) {
context.exeInfo.forcePush = true;
}
context.exeInfo.deferredInitCallback = onSuccess;
await syncCurrentCloudBackend(context);
return await context.amplify.pushResources(context);
} catch (e) {
console.log(e);
edwardfoyle marked this conversation as resolved.
Show resolved Hide resolved
if (e.name !== 'InvalidDirectiveError') {
const message = e.name === 'GraphQLError' ? e.toString() : e.message;
context.print.error(`An error occurred during the push operation: ${message}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

rootStackId = getRootStackId();
} catch (err) {
return;
}
if (hostedUIProviderCreds) {
stateManager.setDeploymentSecrets(
mergeDeploymentSecrets({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ export function getRootStackId(): string {
const teamProviderInfo = stateManager.getTeamProviderInfo();
const { envName } = stateManager.getLocalEnvInfo();
const envTeamProviderInfo = teamProviderInfo[envName];
if (envTeamProviderInfo && envTeamProviderInfo.awscloudformation) {
const stackId = envTeamProviderInfo.awscloudformation.StackId;
const stackId = envTeamProviderInfo?.awscloudformation?.StackId;
if (typeof stackId === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return stackId.split('/')[2];
}
throw new Error('Root stack Id not found');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -486,12 +486,16 @@ export async function showResourceTable(category, resourceName, filteredResource

table(tableOptions, { format: 'markdown' });

if (tagsUpdated) {
// in the case of a deferred init, need to also check there are resources in the project
// checking for length > 1 because for some reason the aws cfn provider is in the resources list but it's not a resource
const updateTags = tagsUpdated && allResources.length > 1;

if (updateTags) {
print.info('\nTag Changes Detected');
}

const resourceChanged =
resourcesToBeCreated.length + resourcesToBeUpdated.length + resourcesToBeSynced.length + resourcesToBeDeleted.length > 0 || tagsUpdated;
resourcesToBeCreated.length + resourcesToBeUpdated.length + resourcesToBeSynced.length + resourcesToBeDeleted.length > 0 || updateTags;

return resourceChanged;
}
1 change: 1 addition & 0 deletions packages/amplify-cli/src/init-steps/postInitSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export async function postInitSetup(context) {
await context.amplify.pushResources(context);
await runPackage();
} catch (e) {
console.log(e);
edwardfoyle marked this conversation as resolved.
Show resolved Hide resolved
if (e.name !== 'InvalidDirectiveError') {
context.print.error(`An error occurred during the push operation: ${e.message}`);
}
Expand Down
18 changes: 13 additions & 5 deletions packages/amplify-cli/src/init-steps/s9-onSuccess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getProviderPlugins } from '../extensions/amplify-helpers/get-provider-p
import { insertAmplifyIgnore } from '../extensions/amplify-helpers/git-manager';
import { writeReadMeFile } from '../extensions/amplify-helpers/docs-manager';
import { initializeEnv } from '../initialize-env';
import _ from 'lodash';

export async function onHeadlessSuccess(context: $TSContext) {
const frontendPlugins = getFrontendPlugins(context);
Expand Down Expand Up @@ -108,12 +109,19 @@ function generateLocalTagsFile(context: $TSContext) {
}

export function generateAmplifyMetaFile(context: $TSContext) {
if (context.exeInfo.isNewEnv) {
const { projectPath } = context.exeInfo.localEnvInfo;
const { projectPath } = context.exeInfo.localEnvInfo;

stateManager.setCurrentMeta(projectPath, context.exeInfo.amplifyMeta);
stateManager.setMeta(projectPath, context.exeInfo.amplifyMeta);
}
const isNewEnv = context.exeInfo.isNewEnv;
edwardfoyle marked this conversation as resolved.
Show resolved Hide resolved

// store amplifyMeta
const meta = isNewEnv ? {} : stateManager.getMeta(projectPath, { throwIfNotExist: false }) || {};
_.merge(meta, context.exeInfo.amplifyMeta);
stateManager.setMeta(projectPath, meta);

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@edwardfoyle edwardfoyle May 6, 2021

Choose a reason for hiding this comment

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

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

_.merge(currMeta, context.exeInfo.amplifyMeta);
stateManager.setCurrentMeta(projectPath, currMeta);
}

function generateNonRuntimeFiles(context: $TSContext) {
Expand Down
6 changes: 6 additions & 0 deletions packages/amplify-cli/src/initialize-env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ export async function initializeEnv(context: $TSContext, currentAmplifyMeta?: $T

await sequential(categoryInitializationTasks);

// this function can now be called on the push codepath in the case of a deffered root stack push
// in that case, we don't need to push here as that will happen automatically down the road
if (context?.input?.command === 'push') {
return;
}

if (context.exeInfo.forcePush === undefined) {
context.exeInfo.forcePush = await context.amplify.confirmPrompt(
'Do you want to push your resources to the cloud for your environment?',
Expand Down
3 changes: 3 additions & 0 deletions packages/amplify-e2e-core/src/init/deleteProject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return;
}
await retry(
() => describeCloudFormationStack(stackName, region, profileConfig),
stack => stack.StackStatus.endsWith('_COMPLETE'),
Expand Down
12 changes: 12 additions & 0 deletions packages/amplify-e2e-core/src/init/initProjectHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,18 @@ export function amplifyInitSandbox(cwd: string, settings: {}): Promise<void> {
});
}

export function amplifyInitYes(cwd: string): Promise<void> {
return new Promise((resolve, reject) => {
spawn(getCLIPath(), ['init', '--yes'], {
cwd,
stripColors: true,
env: {
CLI_DEV_INTERNAL_DISABLE_AMPLIFY_APP_CREATION: '1',
},
}).run((err: Error) => (err ? reject(err) : resolve()));
});
}

export function amplifyVersion(cwd: string, expectedVersion: string, testingWithLatestCodebase = false): Promise<void> {
return new Promise((resolve, reject) => {
spawn(getCLIPath(testingWithLatestCodebase), ['--version'], { cwd, stripColors: true })
Expand Down
Loading