Deploy option to enable continuous deployment #1745

tophtucker · 2024-10-11T22:34:30Z

Running notes

Whether you build locally or in cloud, the deploy method ends with pollForProcessingCompletion. In the local case, that just means uploading files; in the cloud case, it means doing a whole cloud build. Should we have the same timeout in both cases?
Currently this prompts you to enable continuous deployment even if your project directory is ineligible, so you know it would be an option if the criteria were met, and so we can provide informative errors about why it's not available. Sound good?
Once your continuous deployment setting is saved, do we ever prompt you about it again? Is there a command to turn it on, or do we instruct you to edit deploy.json?
The continuous deployment setting is a property of the source code, but the GitHub connection and build environment are properties of the data app in our database, i.e. the “deploy target”. Those can change independently: e.g. you might unlink GitHub without touching the code, and your next deploy command will fail.
- State like “continuous deployment is enabled but you don’t have GitHub linked”
- Can’t tell when it changed though

… continuousDeployment for the first time

… primary branch in our db to current branch; add some periods to some messages where it seems appropriate for consistency

src/deploy.ts

Co-authored-by: Mike Bostock <[email protected]>

tophtucker · 2024-10-16T14:54:39Z

src/deploy.ts

+    // Disables continuous deployment if there’s no env/source & we can’t link GitHub
+    if (continuousDeployment) continuousDeployment = await this.maybeLinkGitHub(deployTarget);


fil says: if you enable continuous deployment, it should stay on, and if we can't connect to github for whatever reason (you're not in a repo, or no git remote, or no link in our database), the deploy should just fail.

done, and it feels like a much better way to think about it

tophtucker · 2024-10-16T15:11:01Z

src/deploy.ts

+        workspaceLogin: deployTarget.workspace.login,
+        projectSlug: deployTarget.project.slug
+      });
+      if (latestCreatedDeployId !== deployTarget.project.latestCreatedDeployId) {


chatting with fil: there's no guarantee that the new deploy ID is the one you just kicked off; maybe your colleague click the deploy button around the same time. currently, the postProjectBuild method can't return the new deploy ID because it just dispatches a message to the job-manager, which at some point will get around to making a new deploy. two options:

create deploy id earlier, in api, and pass to job manager (which feels like it might mess with our messaging protocol? i don't know all the reasons it was designed like it is today)

pass some unique string to the api, which will pass it to the job manager, which will eventually respond with it, which would let us identify that the deploy was our own

but fil thinks this is probably not a blocking issue with this PR

not planning to do anything about this for now

tophtucker · 2024-10-16T15:11:41Z

src/deploy.ts

-    const deployConfig = await this.getUpdatedDeployConfig();
-    const deployTarget = await this.getDeployTarget(deployConfig);
-    const buildFilePaths = await this.getBuildFilePaths();


fil notes that this could be parallelized: getBuildFilePaths doesn't depend on the previous two; it's filesystem i/o, as opposed to talking to the api. might speed up local deploys a bit

not gonna worry about this for now; also, buildFilePaths is only relevant for the local deploy path, so it's not as clean to parallelize anymore

src/deploy.ts

tophtucker · 2024-10-16T15:40:29Z

src/deploy.ts

+    if (deployTarget.environment.build_environment_id && deployTarget.environment.source) {
+      // can do cloud build
+      return true;


fil notes: if we broke our local git configuration, shouldn't we check it? do we want to check if everything is correct? there's a good chance everything will work, but…

if you have erased your .git folder, or you have changed your remotes, or if you're not on the right branch…

should we check that local and remote git refs match?? the user is expecting a cloud deploy of the same application state they are looking at locally!! if they have local unstaged changes, or are on a different branch, they could get surprising results from doing a cloud build.

if observable cloud is configured to pull from the main branch, and you're on toph/onramp locally, what are you expecting to happen when you run yarn deploy?

and it'll get more confusing with branch previews. how does it work on vercel? on vercel, there's no way to trigger a deploy except to push, so there's no question here. maybe that should be our norm? except you still wanna be able to re-run data loaders, which depend on changing external state not captured by commits.

maybe at a minimum we should just check that the local git repo still exists, still has that remote, and is on the same branch. (i.e. move all the tests in the "else" block up above the if/else.)

should also verify that the repo hasnt been unlinked on the web side

done — now validates repo every time and checks that local and remote repos match. i don't check refs yet; not planning to do it in this PR.

tophtucker · 2024-10-16T15:46:49Z

src/deploy.ts

+      return true;
+    } else {
+      // We only support cloud builds from the root directory so this ignores this.deployOptions.config.root
+      const isGit = existsSync(".git");


todo: log cwd when running yarn deploy. you can run yarn deploy from a child directory like src, but i think it still runs in the context of the root directory, in which case this is correct.

tried logging cwd out of curiosity and indeed, as one would hope/expect, yarn deploy runs in the root directory no matter where you call it from.

tophtucker · 2024-10-16T15:49:37Z

src/deploy.ts

+      // We only support cloud builds from the root directory so this ignores this.deployOptions.config.root
+      const isGit = existsSync(".git");
+      if (isGit) {
+        const remotes = (await promisify(exec)("git remote -v", {cwd: this.deployOptions.config.root})).stdout


note that in loader.ts we make our promises "by hand" (and with spawn instead of exec), but in create.ts we already use promisify, so… seems fine!

tophtucker · 2024-10-16T15:51:52Z

src/deploy.ts

+          .split("\n")
+          .filter((d) => d)
+          .map((d) => d.split(/\s/g));
+        const gitHub = remotes.find(([, url]) => url.startsWith("https://github.com/"));


fil instead has, i think, the SSH case, which we should also check for:

❯ git remote -v observable [email protected]:observablehq/pangea.git (fetch) observable [email protected]:observablehq/pangea.git (push) origin [email protected]:Fil/pangea.git (fetch) origin [email protected]:Fil/pangea.git (push)

tophtucker · 2024-10-16T15:52:38Z

src/deploy.ts

+          .map((d) => d.split(/\s/g));
+        const gitHub = remotes.find(([, url]) => url.startsWith("https://github.com/"));
+        if (gitHub) {
+          const repoName = formatGitUrl(gitHub[1]);


formatGitUrl will also be different in the SSH case

src/deploy.ts

tophtucker · 2024-10-16T15:56:23Z

src/deploy.ts

+          if (authedRepo) {
+            // Set branch to current branch
+            const branch = (
+              await promisify(exec)("git rev-parse --abbrev-ref HEAD", {cwd: this.deployOptions.config.root})


i think cwd may be wrong here, it should be running in the normal cwd of the command, at the root, since we don't support non-root repos/projects

i don't think it matters for this command but it was unnecessary and conceptually wrong so i removed the cwd option

tophtucker · 2024-10-16T15:58:29Z

src/deploy.ts

+            }
+          }
+        } else {
+          this.effects.clack.log.error("No GitHub remote found; cannot enable continuous deployment.");


these should exit with a CliError rather than just logging a message and continuing on with a local deploy

done. overall, after fil's feedback in last review, i adopted a philosophy of: when a setting is set, don't change it back just because it's not working; instead, exit with an informative error.

tophtucker · 2024-10-17T15:54:40Z

src/deploy.ts

+          `Do you want to enable continuous deployment? ${faint(
+            "This builds in the cloud and redeploys whenever you push to this repository."


Dunstan says this could note upfront that continuous deployment requires a GitHub repository

…ix prettttier thing i missed

…ws compatibility

…lling back other forcing things to see if i still need them

…ck up to status quo ante; fix polling for auth to respect deployOptions.deployPollInterval, though one could argue its now too narrowly named

… log junk

…stuff isnt isolated so running the tests changed my email and name. happy to have you as a collaborator, Observable User. removed --global flag... duh...

…init FIRST, duh duh duh

…ult branch after repo is already made, which of course does nothing for the current repo

tophtucker · 2024-11-08T04:03:52Z

I think this is ready for review again!

Purpose

The idea is that, when a user runs yarn deploy, they’re given the option to enable continuous deployment (persisted in deploy.json). If they do, we link the repo and immediately kick off a cloud build; the command exits when the build completes (w/ a link to the deployed app) or fails, same as with a local build. Thereafter, users shouldn’t really have to run yarn deploy anymore, since they can just push to GitHub — but if they do run yarn deploy again (e.g. to pick up new data), it kicks off another cloud build.

Note that the deploy command ensures you’re currently in the same repo and branch that’s configured in our database. (We could go further and add some info to the API response so we can check that the refs match.) The idea is that you’re trying to deploy the state of the repo you’re in, not just controlling the data app management console from a command line in a repo in some different state.

Note that currently there’s no CLI command to change the continuousDeployment setting once set; Fil suggested we could have a separate command to review all settings in deploy.json, but I’m inclined to save that for another PR.

Implementation

When run manually from the command line (as opposed to in our cloud build environment where we pass in an existing deploy ID), running deploy calls startNewDeploy. It calls getDeployTarget to find out where it’s deploying to; in this PR, I’ve updated getDeployTarget to check the continuousDeployment option; if it’s not present, we prompt the user whether to enable it. Back in startNewDeploy, if continuousDeployment is on, we call validateGitHubLink and then cloudBuild; else we build locally and deploy as usual. (validateGitHubLink checks that you have an authorized and matching repo linked in our database; if not, and you’re in a repo with a GitHub remote, we link it, or prompt you to go to the web (to a new screen added in https://github.com/observablehq/observablehq/pull/18314) to authorize it if necessary.)

This adds dependencies on a couple API routes:

/cli/github/repository?owner=${props.ownerName}&repo=${props.repoName}, to check the authorization of a single repository
/cli/project/${id}/environment, to set a GitHub link; this endpoint could potentially be rolled into the post project edit endpoint
/cli/project/${id}/build, to initiate a cloud build & deploy of a project

Fil · 2024-11-08T08:55:17Z

docs/deploying.md


-After deploying an app manually at least once, Observable can handle subsequent deploys for you automatically. You can automate deploys both [on commit](https://observablehq.com/documentation/data-apps/github) (whenever you push a new commit to your project’s default branch) and [on schedule](https://observablehq.com/documentation/data-apps/schedules) (such as daily or weekly).
+<!-- TODO: decide whether to primarily use “continuous deployment” or “automatic deploys” -->


I believe CD is the accepted industry term

Fil

review and suggestions #1805

Fil · 2024-11-08T14:13:42Z

I wonder if we could/should provide a post-commit or post-push hook that would remind the user that their push is going to trigger a build (and where to see the logs etc). (I was just editing a project and could not remember if I had already enabled CD on it.)

* documentation * reformat and type * simpler * clarify the logic in validateGitHubLink fixes an uncaught Error ('Setting source repository for continuous deployment failed' was never reached)

Fil · 2024-11-12T16:55:32Z

src/deploy.ts

+  // the root directory.
+  private async validateGitHubLink(deployTarget: DeployTargetInfo): Promise<void> {
+    if (deployTarget.create) throw new Error("Incorrect deploy target state");
+    if (!deployTarget.project.build_environment_id) throw new CliError("No build environment configured.");


Here we could add a link to the build settings page maybe?

something like https://observablehq.com/projects/@{user}/{data-app-slug}/settings

Fil

Can I undo my "Request changes" status without giving approval?

Edit: NO.

mbostock · 2024-11-12T17:28:30Z

@Fil You can dismiss your review.

obsolete

tophtucker added 7 commits October 4, 2024 02:36

checkpoint - kicking off cloud build from cli sorta working

adaa867

correct ellipsis

fa5e762

continuousDeployment flag persisted in deploy.json

6e614ca

poll for repo access

a7efd4c

cleanup. remove "Do you wanna link GitHub" prompt, since you have to

1077838

various post-demo cleanup; maybeLinkGitHub even if we’re not enabling…

44b1e5f

… continuousDeployment for the first time

better handling of the case where github is not connected at all; set…

b6c2a3f

… primary branch in our db to current branch; add some periods to some messages where it seems appropriate for consistency

mbostock reviewed Oct 15, 2024

View reviewed changes

src/deploy.ts Outdated Show resolved Hide resolved

mbostock reviewed Oct 15, 2024

View reviewed changes

src/deploy.ts Outdated Show resolved Hide resolved

tophtucker and others added 3 commits October 15, 2024 12:09

throw Error → throw new Error

bb790ef

Co-authored-by: Mike Bostock <[email protected]>

Merge branch 'main' into toph/onramp

93203ca

fix existing tests

8128173