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

Deploy cmd #42

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
18deca4
chore: add prettier style as usual for graasp apps
Jul 9, 2020
d248ef0
feat: add basic new command
Jul 9, 2020
70c76dc
feat: load dev env and deploy to aws
Jul 10, 2020
305efa0
refactor: rm test file and rm obsolet promisify()
Jul 10, 2020
53925c3
refactor: remove accidentally added bash script
Jul 10, 2020
68fb33b
feat: add options and validation
Jul 10, 2020
00f48a7
feat: invalidate cache after deployment
Jul 10, 2020
8ba8262
fix: allow console output in eslint
Jul 11, 2020
96d3531
feat: add a fancy progress bar
Jul 11, 2020
7ac7faa
fix: tag validation and accidental async
Jul 11, 2020
dc383ad
chore: add comment to RegExp and backshlashes
Jul 11, 2020
c37afb8
refactor: rename varIsDefined() to isDefined()
Jul 11, 2020
305cb3b
refactor: remove unused env varas from import
Jul 11, 2020
19eb223
refactor: replace unnamed func with arrow func
Jul 11, 2020
820b5f5
fix: minor changes based on feedback on pr
Jul 11, 2020
ad0f6f6
chore: remove obsolete comment
Jul 11, 2020
5687b3b
refactor: factor validation out
Jul 11, 2020
cb5650a
fix: use proper tag validation
Jul 17, 2020
6b38e41
chore: add explanation to regexp
Jul 17, 2020
f4b79ef
fix: remove default env
Jul 17, 2020
3cf0366
refactor: replace isdefined() with lodash
Jul 17, 2020
e85ac36
chore: enable linting warning for console log
Jul 18, 2020
baaa792
Merge branch 'master' of github.com:graasp/graasp-cli into deploy-cmd
Jul 26, 2020
2338ead
chore: try out promisify
Jul 26, 2020
4491092
chore: force commit with eslint errors
Jul 26, 2020
62ea0f8
feat: load sync aws credentials
Jul 26, 2020
89e8821
chore: update flag descriptions
Jul 26, 2020
7a288e8
chore: assemble error outputs in together
Jul 26, 2020
28a0dc5
chore: remove obsolete default env variable
Jul 26, 2020
cfa7135
Merge pull request #45 from graasp/promisify
ugGit Jul 26, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"allow": ["_id"]
}
],
"import/no-named-as-default": 0
"no-console": "off",
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to turn this off, as it is a warning. I like to keep this on b/c it reminds me that we should use a logger, but for now it's fine. There are very nice loggers for CLIs, but you can leave it off for now if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered here #42 (comment)

"import/no-named-as-default": "off"
juancarlosfarah marked this conversation as resolved.
Show resolved Hide resolved
}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"@babel/polyfill": "7.8.7",
"aws-sdk": "2.713.0",
"bson-objectid": "1.3.0",
"cli-progress": "3.8.2",
"del": "4.1.1",
"dotenv": "8.2.0",
"execa": "1.0.0",
Expand Down
5 changes: 5 additions & 0 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,8 @@ export const AWS_SECRET_ACCESS_KEY_LENGTH = 40;

// file names
export const GRAASP_IGNORE_FILE = '.graaspignore';

// deploy settings
export const DEFAULT_BUILD_DIR = 'build/';
export const DEFAULT_APP_VERSION = 'latest';
export const DEFAULT_ENV = '.env.dev';
Copy link
Member

Choose a reason for hiding this comment

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

Why is dev default?

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, please consider this comment.

Copy link
Contributor Author

@ugGit ugGit Jul 17, 2020

Choose a reason for hiding this comment

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

@juancarlosfarah Good question. As this was not the case in the previous deploy.sh script, I will remove this default option. Should I make the flag required as a consequence?

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 just saw that this can't be enforced. So, I suggest to simply let the validation error to pop up.

31 changes: 24 additions & 7 deletions src/createCli.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import yargs from 'yargs';
import prompt from './prompt';
import deploy from './deploy';
import { DEFAULT_STARTER } from './config';
import {
DEFAULT_STARTER,
DEFAULT_BUILD_DIR,
DEFAULT_APP_VERSION,
DEFAULT_ENV,
} from './config';

const createCli = (argv) => {
const cli = yargs();
Expand Down Expand Up @@ -53,14 +58,26 @@ const createCli = (argv) => {
})
.command({
command: 'deploy',
desc: 'Deploy the Graasp app',
desc: 'Deploy a Graasp app to AWS',
builder: (_) =>
_.option('p', {
alias: 'path',
_.option('t', {
alias: 'tag',
type: 'string',
default: '.',
describe: 'Path to the Graasp app that shall be deployed',
}),
default: DEFAULT_APP_VERSION,
describe: 'Tag the deployment with a version',
})
.option('e', {
alias: 'env',
type: 'string',
default: DEFAULT_ENV,
describe: 'Environment used to load variables from',
Copy link
Member

Choose a reason for hiding this comment

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

Environment file used

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, please consider this comment.

})
.option('b', {
alias: 'build',
type: 'string',
default: DEFAULT_BUILD_DIR,
describe: 'Path to the build directory that is deployed',
Copy link
Member

Choose a reason for hiding this comment

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

will be deployed

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, please consider this comment.

}),
handler: deploy,
})
.wrap(cli.terminalWidth())
Expand Down
159 changes: 126 additions & 33 deletions src/deploy.js
Original file line number Diff line number Diff line change
@@ -1,51 +1,109 @@
import AWS from 'aws-sdk';
Copy link
Member

Choose a reason for hiding this comment

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

Let's stick to either aws or Aws.

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 to aws

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, I don't see the change.

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 just pushed the latest changes. Please check again

import s3 from 's3-node-client';
import dotenv from 'dotenv';
import fs from 'fs';
import cliProgress from 'cli-progress';

const path = require('path');
const validateTag = (tag) => {
// Both compilation hints because of backslashes used in RegExp but unecessary by conception in JS Strings
// prettier-ignore
// eslint-disable-next-line no-useless-escape
const pattern = new RegExp('v\\d+(\.\\d+){0,2}$');
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this patter is the same as the one in the bash file. Consider this one:

// matches v<major>.<minor>.<patch>[-<meta>] version format
^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(\-[0-9A-Za-z]*)?$

Copy link
Member

Choose a reason for hiding this comment

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

Note that I haven't checked if it's JS compliant. Might be good to add some basic tests.

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, please consider this comment.

if (tag === 'latest' || pattern.test(tag)) {
console.log(`info: validated tag ${tag}`);
return true;
}
console.error(`error: unable to validate version '${tag}'`);
Copy link
Member

Choose a reason for hiding this comment

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

For these, no need to include error:, info:, etc. That was for the bash logs. Here you can use console.error, console.info, etc.

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, please consider this comment.

return false;
};

// default build directory
const BUILD = 'build/';
const validateEnv = (env) => {
if (fs.existsSync(env)) {
console.log(`info: validated environment file ${env}`);
return true;
}
console.log(`error: environment file '${env}' does not exist`);
return false;
};

/**
* Returns an object with all variables loaded from a environment
* @param {string} environmentName is the suffix after .env.*
*/
const validateBuild = (build) => {
if (fs.existsSync(build)) {
console.log(`info: validated build directory ${build}`);
return true;
}
console.log(`error: build directory '${build}' does not exist`);
return false;
};

const deploy = async (opts) => {
// const { path } = opts;
const isDefined = (variable) => {
Copy link
Member

Choose a reason for hiding this comment

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

Use lodash instead of your own function.

return typeof variable !== 'undefined';
};

const usageMessage = console.log(
'usage: $0 [-e <path/to/file>] [-v <version string>] [-b <path/to/build>]',
);
const deploy = async (opts) => {
const { tag, env, build } = opts;

console.log(`Exectued with path: ${opts.path}`);
console.log(usageMessage);
// Validate command options
if (!validateTag(tag) || !validateEnv(env) || !validateBuild(build)) {
console.error('Abort...');
Copy link
Member

Choose a reason for hiding this comment

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

aborting deployment...

return false;
}

// fetch environment variables
dotenv.config({ path: path.resolve(process.cwd(), '.env.dev') });
/* eslint-disable no-unused-vars */
// dotenv.config({ path: path.resolve(process.cwd(), '.env.dev') });
dotenv.config({ path: env });
const {
REACT_APP_GRAASP_DEVELOPER_ID,
REACT_APP_GRAASP_APP_ID,
REACT_APP_GRAASP_DOMAIN,
REACT_APP_HOST,
REACT_APP_VERSION,
REACT_APP_BASE,
NODE_ENV,
BUCKET,
AWS_DEFAULT_REGION,
AWS_ACCESS_KEY_ID,
AWS_SECRET_ACCESS_KEY,
DISTRIBUTION,
} = process.env;
/* eslint-enable no-unused-vars */

AWS.config.getCredentials(function (err) {
if (err) console.error(err.stack);
// credentials not loaded
else {
console.log('Access key:', AWS.config.credentials.accessKeyId);
// ensure the correct app variables are defined
if (
!isDefined(REACT_APP_HOST) ||
juancarlosfarah marked this conversation as resolved.
Show resolved Hide resolved
!isDefined(REACT_APP_GRAASP_DEVELOPER_ID) ||
!isDefined(REACT_APP_GRAASP_APP_ID)
) {
console.error(
Copy link
Member

Choose a reason for hiding this comment

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

No need to separate into three messages. Just one console.error. And you can remove the error: bit, which was for the bash script.

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, you forgot to remove the error: bit.

'error: environment variables REACT_APP_GRAASP_APP_ID, REACT_APP_GRAASP_DEVELOPER_ID and/or REACT_APP_HOST are not defined',
);
console.error(
'error: you can specify them through a .env file in the app root folder',
);
console.error('error: or through another file specified with the -e flag');
return false;
}

// ensure the correct aws credentials are defined
if (
!isDefined(BUCKET) ||
juancarlosfarah marked this conversation as resolved.
Show resolved Hide resolved
!isDefined(AWS_ACCESS_KEY_ID) ||
!isDefined(AWS_SECRET_ACCESS_KEY)
) {
console.error(
'error: environment variables BUCKET, AWS_ACCESS_KEY_ID and/or AWS_SECRET_ACCESS_KEY are not defined',
);
console.error(
'error: make sure you setup your credentials file correctly using the scripts/setup.sh script',
);
console.error(
'error: and contact your favourite Graasp engineer if you keep running into trouble',
);
}

console.log(
`info: publishing app ${REACT_APP_GRAASP_APP_ID} version ${REACT_APP_VERSION}`,
);

// configure the deployment
Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, I think that you can use this promise version of getting the credentials.

https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Credentials.html#getPromise-property

AWS.config.getCredentials((err) => {
Copy link
Member

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 promisify and await here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juancarlosfarah Absolutely correct. But currently not enough time to include it in this PR. Consequently, the commands risk not to be executed in the correct chronological order.

if (err) {
// credentials not loaded
console.error(err.stack);
}
});

Expand All @@ -54,7 +112,7 @@ const deploy = async (opts) => {
const client = s3.createClient({ s3Client: new AWS.S3() });

const params = {
localDir: BUILD,
localDir: build,
Copy link
Member

Choose a reason for hiding this comment

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

Did not see comment on left side for the delete removed comments:

Put both lines of comment above deleteRemoved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juancarlosfarah Please let me know if here is still something pending

deleteRemoved: true, // default false, whether to remove s3 objects
Copy link
Member

Choose a reason for hiding this comment

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

Put both lines of comment above deleteRemoved.

// that have no corresponding local file.

Expand All @@ -65,21 +123,56 @@ const deploy = async (opts) => {
// See: http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#putObject-property
},
};
const progressBar = new cliProgress.SingleBar(
{},
cliProgress.Presets.shades_classic,
);
const uploader = client.uploadDir(params);
uploader.on('error', function (err) {
uploader.on('error', (err) => {
console.error('unable to sync:', err.stack);
});
uploader.on('progress', function () {
console.log('progress', uploader.progressAmount, uploader.progressTotal);
uploader.on('progress', () => {
progressBar.start(uploader.progressTotal, 0);
progressBar.update(uploader.progressAmount);
});
uploader.on('end', function () {
console.log('done uploading');
uploader.on('end', () => {
progressBar.stop();
// TODO: insert here code that should be executed once the upload is done
Copy link
Member

Choose a reason for hiding this comment

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

This should be done. You can always create helper functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juancarlosfarah Yes, you are right. But I would require more time to do this. Might consider implementing this when finalizing the deploy command.

// e.g. invalidate cache
});

console.log(
`published app to https://${REACT_APP_HOST}/${APP_PATH}/index.html`,
`info: published app to https://${REACT_APP_HOST}/${APP_PATH}/index.html`,
Copy link
Member

Choose a reason for hiding this comment

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

No need for info.

);

// ensure the correct distribution variables are defined
if (!isDefined(DISTRIBUTION)) {
console.error('error: environment variable DISTRIBUTION is not defined');
console.error(
'error: contact your favourite Graasp engineer if you keep running into trouble',
);
return false;
}

// invalidate cloudfront distribution
const pathsToInvalidate = [`/${APP_PATH}/*`];
const invalidationParams = {
DistributionId: DISTRIBUTION,
InvalidationBatch: {
CallerReference: new Date().toString(),
Paths: {
Quantity: pathsToInvalidate.length,
Items: pathsToInvalidate,
},
},
};
const cloudfront = new AWS.CloudFront();
cloudfront.createInvalidation(invalidationParams, (err, data) => {
if (err) console.log(err, err.stack);
Copy link
Member

Choose a reason for hiding this comment

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

Always use {} for if statements, especially with else.

Copy link
Member

Choose a reason for hiding this comment

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

Also, put comments above.

Copy link
Member

Choose a reason for hiding this comment

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

@ugGit, please consider this comment.

// an error occurred
else console.log(data); // successful response
});

return true;
};

Expand Down
Loading