Deploy cmd #42

ugGit · 2020-07-09T14:51:15Z

Which works fine if the command is executed in the project roots directory. E.g.

uchendu@uchendu-HP-ZBook-14:~/Desktop/graasp-cli$ node ~/Desktop/graasp-cli/lib/ deploy -p as
path passed 'as'
/home/uchendu/Desktop/graasp-cli
anay

juancarlosfarah

Nice PR @ugGit. Please have a look at my feedback.

juancarlosfarah · 2020-07-09T15:07:44Z

src/createCli.js

+          type: 'string',
+          describe: 'Path to the Graasp app that shall be deployed',
+        }),
+      handler: promisify(deploy),


Do you need to promisify? The function is already a promise. Just double-checking.

@juancarlosfarah I'm actually not sure. This is inspired by the implementation of the new command. But I tested now and the promisify() could be omitted in both cases. Please let me know if I'm missing a point here, otherwise I'll remove it in the next PR.

Maybe originally the function was not a promise, thus promisify was needed. Looks like it's not needed anymore.

src/createCli.js

juancarlosfarah · 2020-07-09T15:10:29Z

src/deploy.js

+  console.error(`path passed '${path}'`);
+  console.log(process.cwd());
+  try {
+    await spawn('./src/test.sh');


This should be ./test.sh after compiling inside the lib folder.

@juancarlosfarah the test.sh file is not copied into the lib folder. I suppose babel is only considering .js file during compilation.
But even if I move the file manually, the command is executed at the location of the terminal and the path interpreted as relative path. Hence, the file would only be found when the command is executed in the lib folder

juancarlosfarah

Hey @ugGit, I really like this! I think this will make things so much easier to deploy. Thanks for the initiative. I left you some minor comments and questions. Please have a look. @hasanagh, copying you for awareness.

juancarlosfarah · 2020-07-10T12:44:24Z

src/deploy.js

@@ -1,15 +1,85 @@
-import { spawn } from './utils';
+import AWS from 'aws-sdk';


Let's stick to either aws or Aws.

Changed to aws

@ugGit, I don't see the change.

I just pushed the latest changes. Please check again

juancarlosfarah · 2020-07-10T12:44:56Z

src/deploy.js

+import s3 from 's3-node-client';
+import dotenv from 'dotenv';
+
+const path = require('path');


Why use require and not import?

pathis not used anymore in the latest changes.

juancarlosfarah · 2020-07-10T12:45:55Z

src/deploy.js

-  }
+  // const { path } = opts;
+
+  const usageMessage = console.log(


I don't think we need this anymore.

juancarlosfarah · 2020-07-10T12:46:18Z

src/deploy.sh

@@ -0,0 +1,125 @@
+#!/bin/bash


Do we still need this file?

I have removed it.

juancarlosfarah · 2020-07-10T12:47:16Z

src/utils.js

@@ -1,5 +1,7 @@
 import execa from 'execa';

+// TODO: this is not optimal naming since it may cause confusion with 'spawn()' from 'child_process' module;


Refactor if needed to something like spawnProcess in a future commit.

juancarlosfarah · 2020-07-10T12:51:02Z

src/deploy.js

+const path = require('path');
+
+// default build directory
+const BUILD = 'build/';


Two options:

Make default -p be ./build and -p be described as path/to/build/folder that will be deployed.

Leave -p as is and allow for overriding this with a -b.

I think that 1 might be cleaner, but I'm up for discussing this.

Good point. Currently I changed it to -b. This keeps the syntax from the original deploy script which might be appreciated by users of the script. Furthermore, i factored the default value out into the config.js file.

As the changes are easy to make, let me know if you prefer the -p variant.

juancarlosfarah · 2020-07-10T12:51:30Z

src/deploy.js

+  );
+
+  console.log(`Exectued with path: ${opts.path}`);
+  console.log(usageMessage);


I don't think we need this.

juancarlosfarah · 2020-07-10T12:53:27Z

src/deploy.js

+    'usage: $0 [-e <path/to/file>] [-v <version string>] [-b <path/to/build>]',
+  );
+
+  console.log(`Exectued with path: ${opts.path}`);


Typo in executed. Also, we use all small caps in logs. I would suggest:

deploying app in {...} directory

or something similar.

juancarlosfarah · 2020-07-10T12:54:46Z

src/deploy.js

+    REACT_APP_HOST,
+    REACT_APP_VERSION,
+    REACT_APP_BASE,
+    NODE_ENV,


Do you need NODE_ENV?

I wasn't sure yet. It's removed now.

juancarlosfarah · 2020-07-10T12:55:30Z

src/deploy.js

+  } = process.env;
+  /* eslint-enable no-unused-vars */
+
+  AWS.config.getCredentials(function (err) {


Are you using this? Maybe promisify in order not to have to shove everything inside the callback.

Yes. This reloads the credentials from the .env.xxx set before. Hence, the below dev note in the sdk doc does not apply.

Note: If you configure the SDK with static or environment credentials, the credential data should already be present in credentials attribute. This method is primarily necessary to load credentials from asynchronous sources, or sources that can refresh credentials periodically.

However, I'm still looking for a proper way to refactor this parts and maybe promisify will help me to do so. The challenge thereby is to check for the error and stop the further promise chain execution properly...

By rethinking the look we already assure that the configuration can be loaded since we're manually checking that the env variables (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY) exist. I will implement a first draft that we might discuss during the next meeting.

hasanagh · 2020-07-10T13:25:04Z

Hey @ugGit, I really like this! I think this will make things so much easier to deploy. Thanks for the initiative. I left you some minor comments and questions. Please have a look. @hasanagh, copying you for awareness.

@juancarlosfarah Just checked the PR! thanks for copying me.

juancarlosfarah

Hi @ugGit, thanks for the changes. We're almost there! Some minor comments below. Also, I didn't see some of the changes you mentioned, like the aws change. Please have a look.

juancarlosfarah · 2020-07-11T15:50:06Z

src/deploy.js

@@ -1,15 +1,85 @@
-import { spawn } from './utils';
+import AWS from 'aws-sdk';


@ugGit, I don't see the change.

juancarlosfarah · 2020-07-11T15:53:05Z

src/deploy.js

+  // 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}$');


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]*)?$

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

@ugGit, please consider this comment.

.eslintrc

juancarlosfarah · 2020-07-11T15:54:49Z

.eslintrc

      }
    ],
-    "import/no-named-as-default": 0
+    "no-console": "off",


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.

Answered here #42 (comment)

juancarlosfarah · 2020-07-11T15:55:14Z

src/config.js

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


Why is dev default?

@ugGit, please consider this comment.

@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?

I just saw that this can't be enforced. So, I suggest to simply let the validation error to pop up.

juancarlosfarah · 2020-07-11T16:03:02Z

src/deploy.js


  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`,


No need for info.

juancarlosfarah · 2020-07-11T16:03:35Z

src/deploy.js

+  };
+  const cloudfront = new AWS.CloudFront();
+  cloudfront.createInvalidation(invalidationParams, (err, data) => {
+    if (err) console.log(err, err.stack);


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

Also, put comments above.

@ugGit, please consider this comment.

juancarlosfarah · 2020-07-11T16:04:02Z

src/deploy.js

+  );
+
+  // configure the deployment
+  AWS.config.getCredentials((err) => {


I think you need to promisify and await here, no?

@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.

juancarlosfarah · 2020-07-11T16:07:31Z

src/deploy.js

-    console.log('done uploading');
+  uploader.on('end', () => {
+    progressBar.stop();
+    // TODO: insert here code that should be executed once the upload is done


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

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

juancarlosfarah · 2020-07-11T16:08:13Z

src/deploy.js

-  console.log(usageMessage);
+  // Validate command options
+  if (!validateTag(tag) || !validateEnv(env) || !validateBuild(build)) {
+    console.error('Abort...');


aborting deployment...

juancarlosfarah

Thanks @ugGit for the changes. I think a bunch of my comments from the previous commits got overridden b/w when I made them and when you made the changes. I have tagged you in some. Please have a look at them. Also, no need to prefix logs with error: or info:. That was for Bash's echo function. Here you can use console.info or console.error, or eventually use a fancier logging tool.

juancarlosfarah · 2020-07-11T21:36:08Z

src/utils.js

  return execa(file, args, opts);
 };
+
+export const isDefined = (variable) => {


There is also a _.isUndefined function that you could use from lodash.

I wasn't aware that this function existed. I have replaced it.

juancarlosfarah · 2020-07-11T21:36:40Z

src/config.js

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


@ugGit, please consider this comment.

juancarlosfarah · 2020-07-11T21:37:49Z

src/deploy.js

+
+  // validate command options
+  if (!validateTag(tag) || !validateEnv(env) || !validateBuild(build)) {
+    console.error('Abort...');


In previous comment I suggest you change this message to aborting deployment.

juancarlosfarah · 2020-07-11T21:38:44Z

src/deploy.js

+  };
+  const cloudfront = new AWS.CloudFront();
+  cloudfront.createInvalidation(invalidationParams, (err, data) => {
+    if (err) console.log(err, err.stack);


@ugGit, please consider this comment.

juancarlosfarah · 2020-07-11T21:39:35Z

src/deploy.js


  // configure the deployment
-  AWS.config.getCredentials((err) => {
+  aws.config.getCredentials((err) => {


Even if you guarantee that the correct variables are present, don't you need to wait for the setup to be done?

Indeed the execution relies currently on a fast execution of this async method. A next PR must handle this problematic and properly chain the execution.

juancarlosfarah · 2020-07-16T19:54:03Z

@ugGit, did you manage to look into the requested changes?

ugGit · 2020-07-17T07:34:49Z

@juancarlosfarah Yes. Apart of the handling for the async calls, all feedback is implemented

juancarlosfarah

Hey @ugGit, I did some more research, and the node S3 client that you have used is pretty outdated:

https://github.com/shreyawhiz/node-s3-client#readme

We should be using everything from AWS where possible and use the promises in order to avoid getting stuck in complex callback situations.

Let me know if you want to discuss this.

juancarlosfarah · 2020-07-22T20:12:59Z

src/deploy.js

+    !isDefined(REACT_APP_GRAASP_DEVELOPER_ID) ||
+    !isDefined(REACT_APP_GRAASP_APP_ID)
+  ) {
+    console.error(


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

juancarlosfarah · 2020-07-22T20:20:15Z

src/deploy.js

+    `publishing app ${REACT_APP_GRAASP_APP_ID} version ${REACT_APP_VERSION}`,
  );

  // configure the deployment


@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

juancarlosfarah · 2020-07-22T20:27:54Z

src/deploy.js


  // ensure the correct distribution variables are defined
-  if (!isDefined(DISTRIBUTION)) {
+  if (_.isUndefined(DISTRIBUTION)) {


everything after line 159 should be inside the on('end', () => { call in order to work. Preferably you can use the promises version of the native s3 client from AWS.

Promisify

Uchendu Nwachukwu added 2 commits July 9, 2020 16:47

chore: add prettier style as usual for graasp apps

18deca4

feat: add basic new command

d248ef0

ugGit marked this pull request as ready for review July 9, 2020 14:51

ugGit requested a review from juancarlosfarah July 9, 2020 14:51

juancarlosfarah requested changes Jul 9, 2020

View reviewed changes

Uchendu Nwachukwu added 2 commits July 10, 2020 14:40

feat: load dev env and deploy to aws

70c76dc

refactor: rm test file and rm obsolet promisify()

305efa0

juancarlosfarah requested changes Jul 10, 2020

View reviewed changes

Uchendu Nwachukwu added 12 commits July 10, 2020 15:33

refactor: remove accidentally added bash script

53925c3

feat: add options and validation

68fb33b

feat: invalidate cache after deployment

00f48a7

fix: allow console output in eslint

8ba8262

feat: add a fancy progress bar

96d3531

fix: tag validation and accidental async

7ac7faa

chore: add comment to RegExp and backshlashes

dc383ad

refactor: rename varIsDefined() to isDefined()

c37afb8

refactor: remove unused env varas from import

305cb3b

refactor: replace unnamed func with arrow func

19eb223

fix: minor changes based on feedback on pr

820b5f5

chore: remove obsolete comment

ad0f6f6

juancarlosfarah requested changes Jul 11, 2020

View reviewed changes

refactor: factor validation out

5687b3b

juancarlosfarah requested changes Jul 11, 2020

View reviewed changes

Uchendu Nwachukwu added 4 commits July 17, 2020 09:09

fix: use proper tag validation

cb5650a

chore: add explanation to regexp

6b38e41

fix: remove default env

f4b79ef

refactor: replace isdefined() with lodash

3cf0366

chore: enable linting warning for console log

e85ac36

juancarlosfarah requested changes Jul 22, 2020

View reviewed changes

Uchendu Nwachukwu and others added 8 commits July 26, 2020 12:33

Merge branch 'master' of github.com:graasp/graasp-cli into deploy-cmd

baaa792

chore: try out promisify

2338ead

chore: force commit with eslint errors

4491092

feat: load sync aws credentials

62ea0f8

chore: update flag descriptions

89e8821

chore: assemble error outputs in together

7a288e8

chore: remove obsolete default env variable

28a0dc5

Merge pull request #45 from graasp/promisify

cfa7135

Promisify

juancarlosfarah self-assigned this Aug 21, 2020

		@@ -1,15 +1,85 @@
		import { spawn } from './utils';
		import AWS from 'aws-sdk';

		@@ -1,5 +1,7 @@
		import execa from 'execa';

		// TODO: this is not optimal naming since it may cause confusion with 'spawn()' from 'child_process' module;

Deploy cmd #42

Are you sure you want to change the base?

Deploy cmd #42

Uh oh!

Conversation

ugGit commented Jul 9, 2020

Uh oh!

juancarlosfarah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ugGit Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juancarlosfarah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ugGit Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasanagh commented Jul 10, 2020

Uh oh!

juancarlosfarah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ugGit Jul 9, 2020 •

edited

Loading

ugGit Jul 11, 2020 •

edited

Loading

ugGit Jul 17, 2020 •

edited

Loading