feat: create deploy before building #7768
Conversation
src/commands/deploy/deploy.ts
Outdated
| reportDeployError({ error_, failAndExit: logAndThrowError }) | ||
|
|
||
| // We'll never get here, as `reportDeployError` calls `logAndThrowError`, | ||
| // which will throw. But we're throwing anyway just to make types happy. | ||
| throw error_ |
There was a problem hiding this comment.
I pushed a type fix to avoid needing this. Hope that's cool 😁.
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
LGTM other than my two comments
| process.env.DEPLOY_ID = deployId | ||
| process.env.DEPLOY_URL = deployUrl |
There was a problem hiding this comment.
🤔 What purpose does populating these in the CLI process serve? If it serves a purpose, could we leave a comment here explaining that? and is it possible to add test coverage?
There was a problem hiding this comment.
This is lets @netlify/config, which runs in the same process, grab the deploy details.
There was a problem hiding this comment.
In that case, what is the purpose of the mutations to command.netlify.cachedConfig.env? and why can't we use the newEnvChanges mechanism? I think the why would be worth documenting in a comment in a follow-up PR!
| @@ -703,9 +718,6 @@ | |||
|
|
|||
| logAndThrowError(`Error while running build${message}`) | |||
| } | |||
| if (exitCode !== 0) { | |||
| exit(exitCode) | |||
| } | |||
There was a problem hiding this comment.
Are these changes related to this PR? I'm not sure I'm following. Could we maybe pull them out into a separate PR for traceability? That would also give us a chance to think through it carefully and add test coverage.
As is, I might be missing something, but it seems like this changes some behaviours. Previously, we called logAndThrowError() (which does exactly what it says on the tin, i.e. it does not exit) in the json case and quietly called exit() instead in the non-json case. After these changes, we always call logAndThrowError() — this changes the output behaviour — and never call exit(). If I'm tracing this right, the thrown error will bubble all the way up to commander's handling. This may all end up resulting in similar behaviour, but it seems risky, especially without thorough coverage.
I assume the reason this change was made is to hit line 1163 in the build failure + non-json case, because we now have an orphaned deploy to clean up?
There was a problem hiding this comment.
I assume the reason this change was made is to hit line 1163 in the build failure + non-json case, because we now have an orphaned deploy to clean up?
Yes. Without this change, handleBuild won't throw, so we can't take any action if the build fails.
I don't think we can move this to a separate PR because cleaning up an orphaned deploy is a critical part of this.
Rather than explicitly calling exit, we now throw the error and let it bubble up.
Can you expand on what behaviours you think this might change?
There was a problem hiding this comment.
I don't think we can move this to a separate PR because cleaning up an orphaned deploy is a critical part of this.
I meant that PR could land first
Can you expand on what behaviours you think this might change?
I was wondering about other, existing cases where await runBuild() might throw, which we previously handled ourselves and exited but would now lead to bubbling the error all the way up to commander, the details of which I'm not intimately familiar with.
but I've just now looked up the docs (for the correct old commander version we're using) and it seems like it should be fine—it should just log the error and exit(1).
Oh, there is a behaviour change here I believe: in the json case, this PR will add new (non-JSON) output (the error message, or rather specifically our custom error output) to stderr. But since I believe that's where we landed as our ideal behaviour for --json (100% JSON on stdout, freeform on stderr), I think we're good? 👍🏼
Currently, the
deploycommand (without--no-build) first builds the site and then does a deploy. This means that the build process won't be able to reference the deploy, because it doesn't exist at that point.This is inconsistent with what happens in our build system and creates compatibility issues with certain features. For example, skew protection only works if the
DEPLOY_IDenvironment variable is populated with the deploy ID, so it currently doesn't work with CLI deploys.This PR changes the order of operations such that we first create a deploy, then we run the build referencing that deploy, and finally update the deploy depending on the result of the build and deploy processes (whether successful or not).