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

feat(cli): cLI updates and new features for Amplify Console #2742

Merged
merged 18 commits into from
Nov 23, 2019

Conversation

UnleashedMind
Copy link
Contributor

Issue #, if available:

Description of changes:

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

@lgtm-com
Copy link

lgtm-com bot commented Nov 14, 2019

This pull request introduces 1 alert when merging 62fa864 into fe0e979 - view on LGTM.com

new alerts:

  • 1 for Assignment to constant

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2019

This pull request introduces 2 alerts when merging df87b2d into fe0e979 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2019

This pull request introduces 2 alerts when merging 1069ea0 into fe0e979 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2019

This pull request introduces 3 alerts when merging 478ccc6 into fe0e979 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Property access on null or undefined

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2019

This pull request introduces 2 alerts when merging fc8b1a8 into 0cd2e90 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2019

This pull request introduces 2 alerts when merging 3ce6695 into 7155787 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@@ -26,10 +26,12 @@ function normalizeKey(key) {
}

function normalizeValue(key, value) {
if (key === 'app') {
return value;
let normalizedValue = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of mutating a variable I suggest we could do this

try { return JSON.parse(value); } catch (e) { // do nothing } return value;

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 can't do it.
Basically what this function does is that, if the import string can be parsed as JSON, it will parse it, else it will return as is.

const projectPath = process.cwd();
const localEnvFilePath = context.amplify.pathManager.getLocalEnvFilePath(projectPath);
if (fs.existsSync(localEnvFilePath)) {
({ defaultEditor } = readJsonFile(localEnvFilePath));
Copy link
Contributor

Choose a reason for hiding this comment

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

return readJsonFile(localEnvFilePath).defaultEditor without creating and assigning

Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

Added comments, mostly about duplicated code.


function constructStatusURL(appId, envName) {
const prodURL = `https://console.aws.amazon.com/amplify/home#/${appId}/YmFja2VuZA/${envName}`; // eslint-disable-line
const betaURL = `https://aws-mobile-apphub-beta.corp.amazon.com/amplify/home#/${appId}/YmFja2VuZA/${envName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

beta URLs should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was only used for Console's beta testing, removed.

module.exports = {
name: 'pull',
run: async context => {
const currentAmplifyMetaFilePath = context.amplify.pathManager.getCurrentAmplifyMetaFilePath(process.cwd());
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have the cwd in the context as currentDirectory and use it from downstream code and not to reach out to process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see issue with using process.cwd() though, and it's very straightforward.

@@ -35,7 +44,11 @@ function updateBackendConfigDependsOn(category, resourceName, attribute, value)

function updateBackendConfigAfterResourceRemove(category, resourceName) {
const backendConfigFilePath = pathManager.getBackendConfigFilePath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we extract this common code that operates on backendConfig and reuse it instead of having copies of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, done.

const inputParams = normalizeInputParams(context);

if (inputParams.appId) {
inputParams.amplify.appId = inputParams.appId;
Copy link
Contributor

Choose a reason for hiding this comment

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

inputParams.amplify always defined?

The repeated code could be simplified:

const moveProperty = (name) => {
  if (inputParams[name]) {
    inputParams.amplify[name] = inputParams[name];
    delete inputParams[name];
  }
};

const properties = ['appId', 'envName', 'no-override']

for (const property of properties) {
  moveProperty(property);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, inputParams.amplify always exists, see the above comment.

We only allow the "outside" amplify parameter for 'appId', 'envName' and 'no-override', others can not just be added under "amplify".

const confirmKeepCodebase = await context.amplify.confirmPrompt.run('Do you want to keep the backend codebase?', true);
if (confirmKeepCodebase) {
context.print.info('');
context.print.success('Backend environment has been successfully pulled and attached to your project.');
Copy link
Contributor

Choose a reason for hiding this comment

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

wording: what do you think about "the" instead of "your"?

Copy link
Contributor

Choose a reason for hiding this comment

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

The suggested wording has not been implemented: "✔ Successfully pulled backend environment '$envname' from the cloud."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

return backendEnvs[0];
}
} else {
const errorMessage = `Found no currently existing backend environment in the amplify project: ${amplifyApp.name}.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wording is decided by the PM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot find backend environment ${inputEnvName} in Amplify Console app: ${amplifyApp.name}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this workflow, the use does not have inputEnvName
I changed it to:
Cannot find any backend environments in the Amplify project ${amplifyApp.name}.;
Let me know if you want to change again.

fs.removeSync(tempDirPath);
}

async function getAwsConfig(context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have this function, can't we reuse it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.
Moved the method to the configuration manager, and reuse it in both init and attach-backend.

const proxyAgent = require('proxy-agent');
const configurationManager = require('../../lib/configuration-manager');

// const betaEndpoint = 'https://ntb76nklh1.execute-api.us-west-2.amazonaws.com/beta';
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -11,7 +11,7 @@
"amplify-category-storage": "1.31.0",
"amplify-codegen": "1.29.0",
"archiver": "^2.1.1",
"aws-sdk": "^2.510.0",
"aws-sdk": "file:../../aws-sdk/",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this once the SDK is published

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

yarn.lock Outdated
@@ -3995,6 +3995,19 @@ aws-sdk@^2.510.0:
uuid "3.3.2"
xml2js "0.4.19"

"aws-sdk@file:aws-sdk":
Copy link
Contributor

Choose a reason for hiding this comment

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

After SDK publish yarn.lock should be refreshed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

.gitignore Outdated
@@ -22,3 +22,4 @@ packages/*/package-lock.json
amplify-integ*/
packages/amplify-storage-simulator/lib
packages/**/reports/junit/*
aws-sdk/
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was only for Console team's beta testing, removed.

packages/amplify-cli/src/commands/push.js Show resolved Hide resolved
Comment on lines 16 to 41
async function getProvider(context, providerPlugins) {
let result;
const providers = [];
const providerPluginList = Object.keys(providerPlugins);
const { inputParams } = context.exeInfo;
if (inputParams && inputParams.amplify && inputParams.amplify.providers) {
inputParams.amplify.providers.forEach(provider => {
provider = normalizeProviderName(provider, providerPluginList);
if (provider) {
providers.push(provider);
}
});
}

if (providers.length === 0) {
if ((inputParams && inputParams.yes) || providerPluginList.length === 1) {
context.print.info(`Using default provider ${providerPluginList[0]}`);
result = providerPluginList[0]; // eslint-disable-line
} else {
const selectProvider = {
type: 'list',
name: 'selectedProvider',
message: 'Select the backend provider.',
choices: providerPluginList,
default: providerPluginList[0],
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this code similar to the code below. Why can't we expose this as an util and use it here?

async function getProviders(context, providerPlugins) {
let providers = [];
const providerPluginList = Object.keys(providerPlugins);
const { inputParams } = context.exeInfo;
if (inputParams && inputParams.amplify && inputParams.amplify.providers) {
inputParams.amplify.providers.forEach(provider => {
provider = normalizeProviderName(provider, providerPluginList);
if (provider) {
providers.push(provider);
}
});
}
if (providers.length === 0) {
if ((inputParams && inputParams.yes) || providerPluginList.length === 1) {
context.print.info(`Using default provider ${providerPluginList[0]}`);
providers.push(providerPluginList[0]);
} else {
const selectProviders = {
type: 'checkbox',
name: 'selectedProviders',
message: 'Select the backend providers.',
choices: providerPluginList,
default: providerPluginList[0],
};
const answer = await inquirer.prompt(selectProviders);
providers = answer.selectedProviders;
}
}
return providers;
}

Comment on lines +39 to +48
function getDefaultEditor(context) {
let defaultEditor;
const projectPath = process.cwd();
const localEnvFilePath = context.amplify.pathManager.getLocalEnvFilePath(projectPath);
if (fs.existsSync(localEnvFilePath)) {
({ defaultEditor } = readJsonFile(localEnvFilePath));
}

return defaultEditor;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

function getDefaultEditor(context) {
let defaultEditor;
const projectPath = process.cwd();
const localEnvFilePath = context.amplify.pathManager.getLocalEnvFilePath(projectPath);
if (fs.existsSync(localEnvFilePath)) {
({ defaultEditor } = readJsonFile(localEnvFilePath));
}
return defaultEditor;
}

Comment on lines +28 to +37
async function getEditor(context) {
let editor;
if (context.exeInfo.inputParams.amplify && context.exeInfo.inputParams.amplify.defaultEditor) {
editor = normalizeEditor(context.exeInfo.inputParams.amplify.defaultEditor);
} else if (!context.exeInfo.inputParams.yes) {
editor = await editorSelection(editor);
}

return editor;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

async function getEditor(context) {
let editor;
if (context.exeInfo.inputParams.amplify && context.exeInfo.inputParams.amplify.defaultEditor) {
editor = normalizeEditor(context.exeInfo.inputParams.amplify.defaultEditor);
} else if (!context.exeInfo.inputParams.yes) {
editor = await editorSelection(editor);
}
return editor;

Comment on lines +28 to +51
async function getFrontendHandler(context, frontendPlugins, suitableFrontend) {
let frontend;
const frontendPluginList = Object.keys(frontendPlugins);
const { inputParams } = context.exeInfo;

if (inputParams && inputParams.amplify && inputParams.amplify.frontend) {
frontend = normalizeFrontendHandlerName(inputParams.amplify.frontend, frontendPluginList);
}

if (!frontend && inputParams && inputParams.yes) {
frontend = 'javascript';
}

if (!frontend) {
const selectFrontendHandler = {
type: 'list',
name: 'selectedFrontendHandler',
message: "Choose the type of app that you're building",
choices: frontendPluginList,
default: suitableFrontend,
};
const answer = await inquirer.prompt(selectFrontendHandler);
frontend = answer.selectedFrontendHandler;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

async function getFrontendHandler(context, frontendPlugins, suitableFrontend) {
let frontend;
const frontendPluginList = Object.keys(frontendPlugins);
const { inputParams } = context.exeInfo;
if (inputParams && inputParams.amplify.frontend) {
frontend = normalizeFrontendHandlerName(inputParams.amplify.frontend, frontendPluginList);
}
if (!frontend && inputParams && inputParams.yes) {
frontend = 'javascript';
}
if (!frontend) {
const selectFrontendHandler = {
type: 'list',
name: 'selectedFrontendHandler',
message: "Choose the type of app that you're building",
choices: frontendPluginList,
default: suitableFrontend,
};
const answer = await inquirer.prompt(selectFrontendHandler);
frontend = answer.selectedFrontendHandler;
}
return frontend;
}

@@ -0,0 +1,109 @@
const fs = require('fs-extra');
Copy link
Contributor

Choose a reason for hiding this comment

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

This is almost similar to the file below. Can we extract out the common part and reuse it in both places

const fs = require('fs-extra');
const sequential = require('promise-sequential');
const { getFrontendPlugins } = require('../../extensions/amplify-helpers/get-frontend-plugins');
const { getProviderPlugins } = require('../../extensions/amplify-helpers/get-provider-plugins');
const gitManager = require('../../extensions/amplify-helpers/git-manager');
const { initializeEnv } = require('../initialize-env');
const { readJsonFile } = require('../../extensions/amplify-helpers/read-json-file');
async function run(context) {
const { projectPath } = context.exeInfo.localEnvInfo;
const { amplify } = context;
const amplifyDirPath = amplify.pathManager.getAmplifyDirPath(projectPath);
const dotConfigDirPath = amplify.pathManager.getDotConfigDirPath(projectPath);
const backendDirPath = amplify.pathManager.getBackendDirPath(projectPath);
const currentBackendDirPath = amplify.pathManager.getCurrentCloudBackendDirPath(projectPath);
fs.ensureDirSync(amplifyDirPath);
fs.ensureDirSync(dotConfigDirPath);
fs.ensureDirSync(backendDirPath);
fs.ensureDirSync(currentBackendDirPath);
// Get current-cloud-backend's amplify-meta
const currentAmplifyMetafilePath = amplify.pathManager.getCurentAmplifyMetaFilePath();
let currentAmplifyMeta = {};
if (fs.existsSync(currentAmplifyMetafilePath)) {
currentAmplifyMeta = readJsonFile(currentAmplifyMetafilePath);
}
const providerPlugins = getProviderPlugins(context);
const providerOnSuccessTasks = [];
const frontendPlugins = getFrontendPlugins(context);
const frontendModule = require(frontendPlugins[context.exeInfo.projectConfig.frontend]);
await frontendModule.onInitSuccessful(context);
generateLocalRuntimeFiles(context);
generateNonRuntimeFiles(context);
context.exeInfo.projectConfig.providers.forEach(provider => {
const providerModule = require(providerPlugins[provider]);
providerOnSuccessTasks.push(() => providerModule.onInitSuccessful(context));
});
await sequential(providerOnSuccessTasks);
await initializeEnv(context, currentAmplifyMeta);
if (!context.parameters.options.app) {
printWelcomeMessage(context);
}
// Exit the process with a success code
// process.exit(0);
}
function generateLocalRuntimeFiles(context) {
generateLocalEnvInfoFile(context);
generateAmplifyMetaFile(context);
}
function generateLocalEnvInfoFile(context) {
const { projectPath } = context.exeInfo.localEnvInfo;
const jsonString = JSON.stringify(context.exeInfo.localEnvInfo, null, 4);
const localEnvFilePath = context.amplify.pathManager.getLocalEnvFilePath(projectPath);
fs.writeFileSync(localEnvFilePath, jsonString, 'utf8');
}
function generateAmplifyMetaFile(context) {
if (context.exeInfo.isNewEnv) {
const { projectPath } = context.exeInfo.localEnvInfo;
const jsonString = JSON.stringify(context.exeInfo.amplifyMeta, null, 4);
const currentBackendMetaFilePath = context.amplify.pathManager.getCurentAmplifyMetaFilePath(projectPath);
fs.writeFileSync(currentBackendMetaFilePath, jsonString, 'utf8');
const backendMetaFilePath = context.amplify.pathManager.getAmplifyMetaFilePath(projectPath);
fs.writeFileSync(backendMetaFilePath, jsonString, 'utf8');
}
}
function generateNonRuntimeFiles(context) {
generateProjectConfigFile(context);
generateBackendConfigFile(context);
generateProviderInfoFile(context);
generateGitIgnoreFile(context);
}
function generateProjectConfigFile(context) {
// won't modify on new env
if (context.exeInfo.isNewProject) {
const { projectPath } = context.exeInfo.localEnvInfo;
const jsonString = JSON.stringify(context.exeInfo.projectConfig, null, 4);
const projectConfigFilePath = context.amplify.pathManager.getProjectConfigFilePath(projectPath);
fs.writeFileSync(projectConfigFilePath, jsonString, 'utf8');
}
}
function generateProviderInfoFile(context) {
const { projectPath } = context.exeInfo.localEnvInfo;
let teamProviderInfo = {};
const providerInfoFilePath = context.amplify.pathManager.getProviderInfoFilePath(projectPath);
if (fs.existsSync(providerInfoFilePath)) {
teamProviderInfo = readJsonFile(providerInfoFilePath);
Object.assign(teamProviderInfo, context.exeInfo.teamProviderInfo);
} else {
({ teamProviderInfo } = context.exeInfo);
}
const jsonString = JSON.stringify(teamProviderInfo, null, 4);
fs.writeFileSync(providerInfoFilePath, jsonString, 'utf8');
}
function generateBackendConfigFile(context) {
if (context.exeInfo.isNewProject) {
const { projectPath } = context.exeInfo.localEnvInfo;
const backendConfigFilePath = context.amplify.pathManager.getBackendConfigFilePath(projectPath);
fs.writeFileSync(backendConfigFilePath, '{}', 'utf8');
}
}
function generateGitIgnoreFile(context) {
if (context.exeInfo.isNewProject) {
const { projectPath } = context.exeInfo.localEnvInfo;
const gitIgnoreFilePath = context.amplify.pathManager.getGitIgnoreFilePath(projectPath);
gitManager.insertAmplifyIgnore(gitIgnoreFilePath);
}
}
function printWelcomeMessage(context) {
context.print.info('');
context.print.success('Your project has been successfully initialized and connected to the cloud!');
context.print.info('');
context.print.success('Some next steps:');
context.print.info('"amplify status" will show you what you\'ve added already and if it\'s locally configured or deployed');
context.print.info('"amplify <category> add" will allow you to add features like user login or a backend API');
context.print.info('"amplify push" will build all your local backend resources and provision it in the cloud');
context.print.info(
'"amplify publish" will build all your local backend and frontend resources (if you have hosting category added) and provision it in the cloud'
);
context.print.info('');
context.print.success('Pro tip:');
context.print.info('Try "amplify add api" to create a backend API and then "amplify publish" to deploy everything');
context.print.info('');
}
module.exports = {
run,
generateLocalEnvInfoFile,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially tried, but there are too many special handlings, doing that will make code maintenance very hard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not saying it should be the same file. But rather move the common pieces in to a distinct function for instance

  1. Have a function to ensure all the directories exists
  2. generateLocalEnvInfoFile
  3. generateNonRuntimeFiles
  4. generateProjectConfigFile
  5. generateGitIgnoreFile
  6. generateBackendConfigFile (with slight refactor to handle both new and existing project)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will address the code duplications.

Comment on lines +63 to +84
function backupAmplifyFolder(context) {
const projectPath = process.cwd();
const amplifyDirPath = context.amplify.pathManager.getAmplifyDirPath(projectPath);
if (fs.existsSync(amplifyDirPath)) {
const backupAmplifyDirPath = path.join(projectPath, backupAmplifyDirName);
fs.moveSync(amplifyDirPath, backupAmplifyDirPath);
}
}

function restoreOriginalAmplifyFolder(context) {
const projectPath = process.cwd();
const backupAmplifyDirPath = path.join(projectPath, backupAmplifyDirName);
if (fs.existsSync(backupAmplifyDirPath)) {
const amplifyDirPath = context.amplify.pathManager.getAmplifyDirPath(projectPath);
fs.removeSync(amplifyDirPath);
fs.moveSync(backupAmplifyDirPath, amplifyDirPath);
}
}

function removeBackupAmplifyFolder() {
const projectPath = process.cwd();
const backupAmplifyDirPath = path.join(projectPath, backupAmplifyDirName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thes could go to thier own module

context.exeInfo = context.amplify.getProjectDetails();
context.exeInfo.inputParams = constructInputParams(context);
context.print.info('');
context.print.info('Pre pull check:');
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be fired as an event and the check should happen in the event handler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently we don't have pre pull event

}

await initializeEnv(context);
await postPullCodeGenCheck(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be an event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree.
But we've decided to leave it as an empty operation for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even so it should be fired as an event which can either be consumed or not. We should start making use of the event system we have built as it would open for other plugin to consume these events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can mark this as todo, currently there is no pre-pull and post-pull events yet.

@ammarkarachi ammarkarachi self-requested a review November 20, 2019 17:56
@@ -11,7 +11,7 @@
"amplify-category-storage": "1.31.0",
"amplify-codegen": "1.29.0",
"archiver": "^2.1.1",
"aws-sdk": "^2.510.0",
"aws-sdk": "file:../../aws-sdk/",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't merge this till sdk gets published and this change is removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

const configurationManager = require('../../lib/configuration-manager');

// const betaEndpoint = 'https://ntb76nklh1.execute-api.us-west-2.amazonaws.com/beta';
const gammaEndpoint = 'https://e3alza85jk.execute-api.us-west-2.amazonaws.com/gamma';
Copy link
Contributor

Choose a reason for hiding this comment

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

same here. We should not publish/merge this till the change is available in prod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved.

}

const defaultOptions = {
region: 'us-west-2',
Copy link
Contributor

Choose a reason for hiding this comment

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

region is hardcoded. Shouldn't we use the project region?

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 only for beta testing, removed.

// ignore missing config
}

let endpoint = gammaEndpoint;
Copy link
Contributor

Choose a reason for hiding this comment

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

lets not have the default, since we do support overriding this value with env vars

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 already removed.

@@ -51,6 +52,9 @@ async function run(context, resourceDefinition) {
}
})
.then(() => postPushGraphQLCodegen(context))
.then(async () => {
await amplifyServiceManager.postPushCheck(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have events for post push in the new plugin platform? Cant we use that instead of doing it 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.

generally, yes.
But this is for migration of projects setup by CLI prior to this update, it has to happen here to insert the appId's into the right places.


return selection;
} else if (apps.length === 1) {
context.print.info(`Found one amplify project '${apps[0].name} / ${apps[0].appId}'`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Found existing amplify project '${apps[0].name} / ${apps[0].appId}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wording is decided by the PM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this wording chosen by the PM? If not would we be having a separate PR for changing the wording?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was chosen in discussions, then he proof read it.
Not sure why this should be changed. The message here is that " there is ONE app" in the account, and prompt the user to confirm to choose it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not handle custom logic for one or multiple Amplify Console apps.

Comment on lines 283 to 299
function getConfiguredS3Client(awsConfig) {
const httpProxy = process.env.HTTP_PROXY || process.env.HTTPS_PROXY;

if (httpProxy) {
aws.config.update({
httpOptions: {
agent: proxyAgent(httpProxy),
},
});
}

return new aws.S3({ ...awsConfig });
}

module.exports = {
run,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using the directly? We have helper methods in aws-utils module to get an S3 client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the existing s3 client implementation in the amplify-service-manager.js line 333 in the storeArtifcatsForAmplifyService method.
I can not use the S3 client directly here, because in its upload files method, it's expecting a project that's already setup by the Amplify CLI and has the teamProviderInfo and LocalEnvInfo etc. In the attach backend workflow, those items are not there and are to be setup by the workflow.
We need refactor the S3 client in the aws-utils to make it more flexible.

Comment on lines 28 to 30
context.print.error(`Input Amplify appId is invalild: ${inputAmplifyAppId}`);
context.print.info(e);
throw e;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we treat all errors the same? What about network errors? Can we just show the error only if the service sends an error saying application is not found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know the error code for that, and there's no documentation.
Will update when it's available.
But this shouldn't happen very often.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you speak to the console team to get the possible error codes. If we can't get this for initial release could you add a todo here and create task for it so that we have some way of tracking this

@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2019

This pull request introduces 2 alerts when merging cbd0c77 into cc96a05 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2019

This pull request introduces 2 alerts when merging b9fb4a9 into cc96a05 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

},
};

function constructStatusURL(appId, envName) {
const prodURL = `https://console.aws.amazon.com/amplify/home#/${appId}/YmFja2VuZA/${envName}`; // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

what is YmFja2VuZA in the url? Would it be same for every customer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it would be. Orangebox automatically resolved the region.

}

function getDefaultURL() {
const prodURL = `https://console.aws.amazon.com/amplify/home#/create`; // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Why eslint disable?

@@ -0,0 +1,63 @@
const inquirer = require('inquirer');
Copy link
Contributor

Choose a reason for hiding this comment

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

most of the code here are identical to

const inquirer = require('inquirer');
const sequential = require('promise-sequential');
const { getProviderPlugins } = require('../../extensions/amplify-helpers/get-provider-plugins');
const { normalizeProviderName } = require('../input-params-manager');
async function run(context) {
const providerPlugins = getProviderPlugins(context);
const providers = await getProviders(context, providerPlugins);
context.exeInfo.projectConfig.providers = providers;
const initializationTasks = [];
providers.forEach(provider => {
const providerModule = require(providerPlugins[provider]);
initializationTasks.push(() => providerModule.init(context));
});
await sequential(initializationTasks);
return context;
}
async function getProviders(context, providerPlugins) {
let providers = [];
const providerPluginList = Object.keys(providerPlugins);
const { inputParams } = context.exeInfo;
if (inputParams && inputParams.amplify && inputParams.amplify.providers) {
inputParams.amplify.providers.forEach(provider => {
provider = normalizeProviderName(provider, providerPluginList);
if (provider) {
providers.push(provider);
}
});
}
if (providers.length === 0) {
if ((inputParams && inputParams.yes) || providerPluginList.length === 1) {
context.print.info(`Using default provider ${providerPluginList[0]}`);
providers.push(providerPluginList[0]);
} else {
const selectProviders = {
type: 'checkbox',
name: 'selectedProviders',
message: 'Select the backend providers.',
choices: providerPluginList,
default: providerPluginList[0],
};
const answer = await inquirer.prompt(selectProviders);
providers = answer.selectedProviders;
}
}
return providers;
}
module.exports = {
run,
};
and
const inquirer = require('inquirer');
const sequential = require('promise-sequential');
const { getProviderPlugins } = require('../../extensions/amplify-helpers/get-provider-plugins');
const { normalizeProviderName } = require('../input-params-manager');
async function run(context) {
const providerPlugins = getProviderPlugins(context);
const { providers: currentProviders } = context.exeInfo.projectConfig;
const selectedProviders = await configureProviders(context, providerPlugins, currentProviders);
const configTasks = [];
const initializationTasks = [];
const onInitSuccessfulTasks = [];
selectedProviders.forEach(provider => {
const providerModule = require(providerPlugins[provider]);
if (currentProviders.includes(provider)) {
configTasks.push(() => providerModule.configure(context));
} else {
initializationTasks.push(() => providerModule.init(context));
onInitSuccessfulTasks.push(() => providerModule.onInitSuccessful(context));
}
});
await sequential(configTasks);
await sequential(initializationTasks);
await sequential(onInitSuccessfulTasks);
return context;
}
async function configureProviders(context, providerPlugins, currentProviders) {
let providers = [];
const providerPluginList = Object.keys(providerPlugins);
const { inputParams } = context.exeInfo;
if (inputParams.amplify.providers) {
inputParams.amplify.providers.forEach(provider => {
provider = normalizeProviderName(provider, providerPluginList);
if (provider) {
providers.push(provider);
}
});
}
if (providers.length === 0) {
if (inputParams.yes || providerPluginList.length === 1) {
context.print.info(`Using default provider ${providerPluginList[0]}`);
providers.push(providerPluginList[0]);
} else {
const selectProviders = {
type: 'checkbox',
name: 'selectedProviders',
message: 'Select the backend providers.',
choices: providerPluginList,
default: currentProviders,
};
const answer = await inquirer.prompt(selectProviders);
providers = answer.selectedProviders;
}
}
return providers;
}
module.exports = {
run,
};

Can we move the common code to a separate module and use it instead of having different copies of the same code

@@ -0,0 +1,109 @@
const fs = require('fs-extra');
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not saying it should be the same file. But rather move the common pieces in to a distinct function for instance

  1. Have a function to ensure all the directories exists
  2. generateLocalEnvInfoFile
  3. generateNonRuntimeFiles
  4. generateProjectConfigFile
  5. generateGitIgnoreFile
  6. generateBackendConfigFile (with slight refactor to handle both new and existing project)

}

await initializeEnv(context);
await postPullCodeGenCheck(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even so it should be fired as an event which can either be consumed or not. We should start making use of the event system we have built as it would open for other plugin to consume these events


return selection;
} else if (apps.length === 1) {
context.print.info(`Found one amplify project '${apps[0].name} / ${apps[0].appId}'`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this wording chosen by the PM? If not would we be having a separate PR for changing the wording?

@@ -49,6 +50,7 @@ async function run(context, resourceDefinition) {
}

await postPushGraphQLCodegen(context);
await amplifyServiceManager.postPushCheck(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets make use of post push events. If we can't get to it now, add a todo

Comment on lines +645 to +652
if (httpProxy) {
awsConfig = {
...awsConfig,
httpOptions: { agent: proxyAgent(httpProxy) },
};
}

return awsConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be part of any loadConfiguration so that proxy is set up everytime

Comment on lines +37 to +44
if (httpProxy) {
aws.config.update({
httpOptions: {
agent: proxyAgent(httpProxy),
},
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If the proxy setup is added to load configuration, we don't need it here and all the other utils will also get proxy support

Comment on lines +5 to +17
const amplifyServiceRegions = [
'us-east-1',
'us-east-2',
'us-west-2',
'eu-west-1',
'eu-west-2',
'eu-central-1',
'ap-northeast-1',
'ap-northeast-2',
'ap-south-1',
'ap-southeast-1',
'ap-southeast-2',
];
Copy link
Contributor

Choose a reason for hiding this comment

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

could we have single mapping file for all the services instead of it being scattered in different files.

Copy link
Contributor

@attilah attilah left a comment

Choose a reason for hiding this comment

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

My comments are addressed, approving, other's comments need to be addressed as well before merge.

@aws-amplify aws-amplify deleted a comment from UnleashedMind Nov 22, 2019
@aws-amplify aws-amplify deleted a comment from UnleashedMind Nov 22, 2019
Copy link
Contributor

@swaminator swaminator left a comment

Choose a reason for hiding this comment

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

Provided a number of language changes.

await postPullCodeGenCheck(context);

if (!inputParams.yes) {
const confirmKeepCodebase = await context.amplify.confirmPrompt.run('Do you want to keep the backend codebase?', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan on modifying this backend? (Y/n/Learn more)
Learn more:
You have a choice to pull the entire backend definition (infrastructure templates and metadata) or only the metadata (the backend config file) required to connect to the backend. If you want to modify or add new categories to your backend choose 'Yes'. If you want to connect to the backend without making any backend changes choose 'No'. If you’re building a mobile and web app in separate repositories, the recommended workflow is to keep the backend definition (the amplify folder) in only one of the repositories and pull the metadata (the aws-exports or amplifyconfiguration.json file) in the second repository to connect to the same backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the words.
"learn more" will be in another PR

const confirmKeepCodebase = await context.amplify.confirmPrompt.run('Do you want to keep the backend codebase?', true);
if (confirmKeepCodebase) {
context.print.info('');
context.print.success('Backend environment has been successfully pulled and attached to your project.');
Copy link
Contributor

Choose a reason for hiding this comment

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

The suggested wording has not been implemented: "✔ Successfully pulled backend environment '$envname' from the cloud."

} else {
removeFolderStructure(context);
context.print.info('');
context.print.success('Backend environment configuration has been successfully pulled.');
Copy link
Contributor

Choose a reason for hiding this comment

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

✔ Successfully pulled backend environment '$envname' from the cloud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

await sequential(initializationTasks);
spinner.succeed('Initialized provider successfully.');
spinner.succeed(isPulling ? 'Successfully pulled your current backend environment.' : 'Initialized provider successfully.');
Copy link
Contributor

Choose a reason for hiding this comment

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

✔ Successfully pulled backend environment '$envname' from the cloud.

@@ -53,9 +58,9 @@ async function initializeEnv(context, currentAmplifyMeta) {
initializationTasks.push(() => providerModule.initEnv(context, amplifyMeta.providers[provider]));
});

spinner.start(`Initializing your environment: ${currentEnv}`);
spinner.start(isPulling ? `Pulling current backend for environment: ${currentEnv}` : `Initializing your environment: ${currentEnv}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Fetching updates to backend environment: ${currentEnv} from the cloud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

environmentName: inputEnvName,
})
.promise();
context.print.info(`Input envName ${inputEnvName} is verified in Amplify service project ${amplifyApp.name}.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Backend environment ${inputEnvName} found in Amplify Console app: ${amplifyApp.name}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

context.print.info(`Input envName ${inputEnvName} is verified in Amplify service project ${amplifyApp.name}.`);
return getBackendEnvironmentResult.backendEnvironment;
} catch (e) {
context.print.error(`Input envName ${inputEnvName} is invalid in Amplify service project ${amplifyApp.name}.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot find backend environment ${inputEnvName} in Amplify Console app: ${amplifyApp.name}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

return selection;
} else if (apps.length === 1) {
context.print.info(`Found one amplify project '${apps[0].name} / ${apps[0].appId}'`);
const confirmMigrateMessage = 'Do you want to choose it?';
Copy link
Contributor

Choose a reason for hiding this comment

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

Confirm the selection? (y/N)

return selection;
} else if (backendEnvs.length === 1) {
context.print.info(`Found one backend environment '${backendEnvs[0].environmentName}'`);
const confirmMigrateMessage = 'Do you want to choose it?';
Copy link
Contributor

Choose a reason for hiding this comment

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

The recommendation was for when the user only has one Amplify backend environment to automatically select it. The language above is different from what I recommended. "Backend environment '$envname' found. Initializing.." and to get rid of the 'Do you want to choose it'

return backendEnvs[0];
}
} else {
const errorMessage = `Found no currently existing backend environment in the amplify project: ${amplifyApp.name}.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot find backend environment ${inputEnvName} in Amplify Console app: ${amplifyApp.name}

@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants