refactor: extract deploy command logic into separate functions #1240

erezrokah · 2020-09-15T17:02:36Z

- Summary

The PR refactors the deploy command code by extracting most of the logic into functions.
Some logic mutates objects so I didn't extract it yet since I wanted the functions to be pure.

- Test plan

Tested manually the different deploy flags, and actually exposed old bugs in the process (which I fixed).

- Description for the changelog

Should not change behaviour

- A picture of a cute animal (not mandatory but encouraged)

🐼

Related to #1236 and #1227 - I initially started looking into those issues and found the code very hard to follow.

src/commands/deploy.js

erezrokah · 2020-09-15T17:03:33Z

src/commands/deploy.js

@@ -16,20 +16,263 @@ const SitesCreateCommand = require('./sites/create')
 const LinkCommand = require('./link')
 const { NETLIFYDEV, NETLIFYDEVLOG, NETLIFYDEVERR } = require('../utils/logo')

+const triggerDeploy = async ({ api, siteId, siteData, log, error }) => {


Extracted from

cli/src/commands/deploy.js

Line 89 in 39da808

try {

erezrokah · 2020-09-15T17:04:11Z

src/commands/deploy.js

+}
+
+const getDeployFolder = async ({ flags, config, site, siteData, log }) => {
+  let deployFolder


Extracted from

cli/src/commands/deploy.js

Line 105 in 39da808

let deployFolder

and

cli/src/commands/deploy.js

Line 125 in 39da808

if (!deployFolder) {

erezrokah · 2020-09-15T17:07:38Z

src/commands/deploy.js

+  return deployFolder
+}
+
+const validateDeployFolder = async ({ deployFolder, error }) => {


Old version was:

cli/src/commands/deploy.js

Line 463 in 39da808

function ensureDirectory(resolvedDeployPath, error) {

Which was broken in multiple places:

Using e.status instead of e.code (fixed in the new version)

cli/src/commands/deploy.js

Line 468 in 39da808

if (e.status === 'ENOENT') {

Referencing stat.isDirectory instead of stat.isDirectory()

cli/src/commands/deploy.js

Line 480 in 39da808

if (!stat.isDirectory) {

🤦‍♂️

erezrokah · 2020-09-15T17:08:11Z

src/commands/deploy.js

+  return stat
+}
+
+const getFunctionsFolder = ({ flags, config, site, siteData }) => {


Extracted from

cli/src/commands/deploy.js

Line 114 in 39da808

let functionsFolder

erezrokah · 2020-09-15T17:08:49Z

src/commands/deploy.js

+  return functionsFolder
+}
+
+const validateFunctionsFolder = async ({ functionsFolder, log, error }) => {


Extracted from

cli/src/commands/deploy.js

Line 114 in 39da808

let functionsFolder

and fixed to reference stat.isDirectory()

erezrokah · 2020-09-15T17:10:00Z

src/commands/deploy.js

+const validateFolders = async ({ deployFolder, functionsFolder, error, log }) => {
+  const deployFolderStat = await validateDeployFolder({ deployFolder, error })
+  const functionsFolderStat = await validateFunctionsFolder({ functionsFolder, error, log })
+  return { deployFolderStat, functionsFolderStat }


We need the return value of validateFunctionsFolder. If it's undefined (since the folder doesn't exist) we don't pass functionsFolder to the js-client.
This was done here:

cli/src/commands/deploy.js

Line 167 in 39da808

functionsFolder = undefined

erezrokah · 2020-09-15T17:10:50Z

src/commands/deploy.js

+  return { deployFolderStat, functionsFolderStat }
+}
+
+const runDeploy = async ({


Extracted from here:

cli/src/commands/deploy.js

Line 181 in 39da808

let results

erezrokah · 2020-09-15T17:12:18Z

src/commands/deploy.js

+  const deployUrl = get(results, 'deploy.deploy_ssl_url') || get(results, 'deploy.deploy_url')
+  const logsUrl = `${get(results, 'deploy.admin_url')}/deploys/${get(results, 'deploy.id')}`
+
+  return {


This information was used here

cli/src/commands/deploy.js

Line 241 in 39da808

const siteUrl = results.deploy.ssl_url || results.deploy.url

and here

cli/src/commands/deploy.js

Line 263 in 39da808

name: results.deploy.deployId,

erezrokah · 2020-09-15T17:12:51Z

src/commands/deploy.js

+  }
+}
+
+const printResults = ({ flags, results, deployToProduction, log, logJson, exit }) => {


Extracted from

cli/src/commands/deploy.js

Line 245 in 39da808

const msgData = {

erezrokah · 2020-09-15T17:14:12Z

src/commands/deploy.js

-        branch: alias,
+    log(
+      prettyjson.render({
+        'Deploy path': deployFolder,


We don't need the conditions here:

cli/src/commands/deploy.js

Line 145 in 39da808

pathInfo['Functions path'] = functionsFolder

and here

cli/src/commands/deploy.js

Line 150 in 39da808

pathInfo['Configuration path'] = configPath

as prettyjson doesn't print undefined values

erezrokah · 2020-09-15T17:14:56Z

src/commands/deploy.js

    }

    const deployToProduction = flags.prod
-    if (deployToProduction && alias) {
-      this.error(`--prod and --alias flags cannot be used at the same time`)


Replaced this with exclusive: ['alias', 'branch'] on the prod flag: https://github.com/netlify/cli/pull/1240/files#diff-e9001bf37e31e33db8a717da3ac59a69R474

ehmicky · 2020-09-16T17:07:41Z

src/commands/deploy.js

+}
+
+const validateFolders = async ({ deployFolder, functionsFolder, error, log }) => {
+  const deployFolderStat = await validateDeployFolder({ deployFolder, error })


Can we run those two async functions in parallel (Promise.all())?

validateFunctionsFolder prints a warning message when the folder doesn't exist, while validateDeployFolder exits.
Didn't want to run into a case where we print a warning then exit with another error.

Makes sense 👍
This is micro-optimization anyway since those I/O calls are quite fast.

ehmicky · 2020-09-16T17:08:53Z

src/commands/deploy.js

+      statusCb: flags.json || flags.silent ? () => {} : deployProgressCb(),
+      draft: !deployToProduction && !alias,
+      message: flags.message,
+      deployTimeout: flags.timeout * 1000 || 1.2e6,


Should we extract that 1.2e6 into a top-level constant variable?

Sure going to update it

Done in f00db3f#diff-e9001bf37e31e33db8a717da3ac59a69R179

ehmicky

Thanks @erezrokah, this is much cleaner! 🎉

refactor: extract deploy command logic into separate functions

ddb387a

erezrokah commented Sep 15, 2020

View reviewed changes

src/commands/deploy.js Outdated Show resolved Hide resolved

erezrokah commented Sep 15, 2020

View reviewed changes

erezrokah marked this pull request as ready for review September 15, 2020 17:15

erezrokah added the type: chore work needed to keep the product and development running smoothly label Sep 15, 2020

refactor: use promisify instead of fs-extra

ef9a68a

This was referenced Sep 15, 2020

Chore: remove fs-extra as a dependecy #1241

Closed

Deploy edge handlers locally #1236

Closed

ehmicky reviewed Sep 16, 2020

View reviewed changes

ehmicky approved these changes Sep 16, 2020

View reviewed changes

erezrokah mentioned this pull request Sep 16, 2020

Feat: edge handlers deploy #1244

Merged

1 task

refactor: extract default timeout to a const

f00db3f

erezrokah merged commit c9ae248 into master Sep 17, 2020

erezrokah deleted the refactor/cleanup_deploy_command branch September 17, 2020 07:07

snyk-bot mentioned this pull request Sep 15, 2021

[Snyk] Fix for 1 vulnerabilities hixio-mh/cli-1#38

Open

refactor: extract deploy command logic into separate functions #1240

refactor: extract deploy command logic into separate functions #1240

Uh oh!

Conversation

erezrokah commented Sep 15, 2020

Uh oh!

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!

erezrokah Sep 15, 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!

erezrokah Sep 15, 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!

erezrokah Sep 16, 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!

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!

ehmicky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erezrokah Sep 15, 2020 •

edited

Loading

erezrokah Sep 15, 2020 •

edited

Loading

erezrokah Sep 16, 2020 •

edited

Loading