-
Notifications
You must be signed in to change notification settings - Fork 402
Feat: edge handlers deploy #1244
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
Conversation
e4ba7b7
to
d6b994d
Compare
@@ -16,8 +14,8 @@ const isObject = require('lodash.isobject') | |||
const SitesCreateCommand = require('./sites/create') | |||
const LinkCommand = require('./link') | |||
const { NETLIFYDEV, NETLIFYDEVLOG, NETLIFYDEVERR } = require('../utils/logo') | |||
|
|||
const statAsync = promisify(fs.stat) | |||
const { statAsync } = require('../lib/fs') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a few places in the code that we used promisify
with fs
.
Moved those to a shared lib
@@ -170,15 +168,27 @@ const runDeploy = async ({ | |||
log('Deploying to draft URL...') | |||
} | |||
|
|||
const draft = !deployToProduction && !alias |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We first create the deploy, then upload edge handlers and finally let js-client
finalise the deploy and upload files and functions.
@@ -212,7 +222,6 @@ const runDeploy = async ({ | |||
const logsUrl = `${get(results, 'deploy.admin_url')}/deploys/${get(results, 'deploy.id')}` | |||
|
|||
return { | |||
name: results.deploy.deployId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
results.deploy.deployId
is always undefined
, this should be either results.deploy.id
or results.deployId
.
Since name
was never printed, and deployId
is already part of the result, I chose to remove the line entirely.
@ehmicky, going to add some tests, but this is ready to review. |
} catch (e) { | ||
updateProgress(spinner, `Failed deploying Edge Handlers: ${e.message}`, logSymbols.error) | ||
try { | ||
await api.cancelSiteDeploy({ deploy_id: deployId }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cleanup can also be moved to the main deploy
function if that makes more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are moving super fast on this! 🎉
69db45f
to
2385078
Compare
@@ -179,21 +197,7 @@ class BaseCommand extends Command { | |||
* @return {[string, string]} - tokenValue & location of resolved Netlify API token | |||
*/ | |||
getConfigToken(tokenFromFlag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remove this since it's used in child classes
@@ -291,4 +295,5 @@ BaseCommand.flags = { | |||
}), | |||
} | |||
|
|||
BaseCommand.getToken = getToken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used in the tests
|
||
const siteName = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved most of the site creating logic into './utils/createLiveTestSite'
if (!spinner) { | ||
return | ||
} | ||
const symbol = error ? logSymbols.error : logSymbols.success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice touch :)
(Hiding logSymbols
behind an error
property)
005d808
to
a64ae73
Compare
a64ae73
to
f057acf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
- Summary
Fixes #1236
This is a draft PR not to be reviewed yet (needs some cleanup and tests).
Opening for visibility.
Depends on netlify/js-client#155 and branched out from #1240
The flow is:
js-client
).netlify/edge-handlers
exists or passed as an argument via--edgeHandlers
deploy
fromjs-client
with the createddeployId
Will add comments after #1240 is merged and I do a rebase.
- Test plan
addons
andenv
).- Description for the changelog
Support deploying Edge Handlers as a part of the deploy command.
- A picture of a cute animal (not mandatory but encouraged)
🐻