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 <mbostock@gmail.com>

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.

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

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

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

src/deploy.ts

+      deployId = await this.createNewDeploy(deployTarget);
+      await this.uploadFiles(deployId, buildFilePaths);
+      await this.markDeployUploaded(deployId);
+    }
    return await this.pollForProcessingCompletion(deployId);


this pollForProcessingCompletion pattern, in the continuous deployment case, keeps the user's terminal busy even though at that point all the work is being done on our servers. i mean, they can always ctrl-c, and we could tell them it's safe to do that. we could return early and say "when your deploy is done, if it succeeds, it will be visible at this url". but maybe it's better to wait so that we can return an error code if it fails?

fil thinks it's correct as-is; if someone has scripted this, they can run yarn deploy to kick off a cloud build, and they will receive a CliError code if the cloud deploy fails.

Given a magic wand, would you want to stream the build logs here?

@mythmon Eventually, yeah!

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

src/deploy.ts

    let deployTarget: DeployTargetInfo;
    if (deployConfig.workspaceLogin && deployConfig.projectSlug) {
      try {
        const project = await this.apiClient.getProject({
          workspaceLogin: deployConfig.workspaceLogin,
          projectSlug: deployConfig.projectSlug
        });
-        deployTarget = {create: false, workspace: project.owner, project};
+        const environment = await this.apiClient.getProjectEnvironment({id: project.id});


at some point ~~(maybe not in this PR)~~ i want to combine getProjectEnvironment into getProject. it could at least be part of that api response; ideally we'd also fold the project_config table into new columns on the projects table.

(currently, getProjectEnvironment has slightly different permissions: it's only accessible by members of the project's workspace who can view the project. for a public project, we should not return those fields. but that would still be doable if it were one table and one endpoint; we'd just serve a redacted version in the public response.)

edit: fil points out that if we don't do it now, we'll have old framework projects in the wild and we'd have to continue supporting the getProjectEnvironment for a long time. so i guess i should do it now!

API side done in https://github.com/observablehq/observablehq/pull/18390; still gotta adopt it here

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

src/deploy.ts

-    await this.markDeployUploaded(deployId);
+  private async maybeLinkGitHub(deployTarget: DeployTargetInfo): Promise<boolean> {
+    if (deployTarget.create) {
+      throw new Error("Incorrect deployTarget state");


fil says: update to something more user-readable

Suggested change

throw new Error("Incorrect deployTarget state");

throw new Error("Inconsistent deploy target state");

they still wouldn't know what to do with that but at least they could send it to us…

tophtucker · 2024-10-16T15:30:16Z

src/deploy.ts

+    if (deployTarget.create) {
+      throw new Error("Incorrect deployTarget state");
+    }
+    if (!this.effects.isTty) return false;


this should move down to the "else" block below. we only need to check tty when we're depending on user interaction. if the build_environment_id and source are already set up just fine, then we should return true even if it's not an interactive terminal.

(fil notes: there's also process.stdout.isTTY, which says if we can write color codes and such.)

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

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.

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 git@github.com:observablehq/pangea.git (fetch) observable git@github.com:observablehq/pangea.git (push) origin git@github.com:Fil/pangea.git (fetch) origin git@github.com: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

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

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

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 <mbostock@gmail.com>

Merge branch 'main' into toph/onramp

93203ca

fix existing tests

8128173

tophtucker commented Oct 16, 2024

View reviewed changes

src/deploy.ts Outdated Show resolved Hide resolved

tophtucker commented Oct 16, 2024

View reviewed changes

tophtucker commented Oct 17, 2024

View reviewed changes

tophtucker added 2 commits October 22, 2024 16:52

start on docs

56e4d14

Merge branch 'main' into toph/onramp

6f7c852

tophtucker added 4 commits October 25, 2024 15:03

use new singular endpoint to see if repo is authed

9e234a2

clean up logic, more dry

c9effe0

rm getProjectEnvironment calls

8c75f52

link to internal interstitial screen instead of directly to github

75cbf50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deploy option to enable continuous deployment #1745

Deploy option to enable continuous deployment #1745

tophtucker commented Oct 11, 2024 •

edited

Loading

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

mythmon Oct 16, 2024

tophtucker Oct 17, 2024

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 25, 2024 •

edited

Loading

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 26, 2024

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

tophtucker Oct 16, 2024

tophtucker Oct 17, 2024

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

	throw new Error("Incorrect deployTarget state");
	throw new Error("Inconsistent deploy target state");

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

Deploy option to enable continuous deployment #1745

Are you sure you want to change the base?

Deploy option to enable continuous deployment #1745

Conversation

tophtucker commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tophtucker Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

tophtucker Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tophtucker Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tophtucker Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tophtucker Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tophtucker commented Oct 11, 2024 •

edited

Loading

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 25, 2024 •

edited

Loading

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 16, 2024 •

edited

Loading

tophtucker Oct 16, 2024 •

edited

Loading