Skip to content
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

Deploy option to enable continuous deployment #1745

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
212 changes: 192 additions & 20 deletions src/deploy.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import {exec} from "node:child_process";
import {createHash} from "node:crypto";
import type {Stats} from "node:fs";
import {existsSync} from "node:fs";
import {readFile, stat} from "node:fs/promises";
import {join} from "node:path/posix";
import {promisify} from "node:util";
import slugify from "@sindresorhus/slugify";
import wrapAnsi from "wrap-ansi";
import type {BuildEffects, BuildManifest, BuildOptions} from "./build.js";
Expand All @@ -20,6 +23,7 @@ import type {
DeployManifestFile,
GetCurrentUserResponse,
GetDeployResponse,
GetProjectEnvironmentResponse,
GetProjectResponse,
WorkspaceResponse
} from "./observableApiClient.js";
Expand All @@ -33,6 +37,10 @@ const DEPLOY_POLL_MAX_MS = 1000 * 60 * 5;
const DEPLOY_POLL_INTERVAL_MS = 1000 * 5;
const BUILD_AGE_WARNING_MS = 1000 * 60 * 5;

export function formatGitUrl(url: string) {
return new URL(url).pathname.slice(1).replace(/\.git$/, "");
}

export interface DeployOptions {
config: Config;
deployConfigPath: string | undefined;
Expand Down Expand Up @@ -82,9 +90,14 @@ const defaultEffects: DeployEffects = {

type DeployTargetInfo =
| {create: true; workspace: {id: string; login: string}; projectSlug: string; title: string; accessLevel: string}
| {create: false; workspace: {id: string; login: string}; project: GetProjectResponse};

/** Deploy a project to ObservableHQ */
| {
create: false;
workspace: {id: string; login: string};
project: GetProjectResponse;
environment: GetProjectEnvironmentResponse;
};

/** Deploy a project to Observable */
export async function deploy(deployOptions: DeployOptions, effects = defaultEffects): Promise<void> {
Telemetry.record({event: "deploy", step: "start", force: deployOptions.force});
effects.clack.intro(`${inverse(" observable deploy ")} ${faint(`v${process.env.npm_package_version}`)}`);
Expand Down Expand Up @@ -190,14 +203,128 @@ class Deployer {
return deployInfo;
}

private async startNewDeploy(): Promise<GetDeployResponse> {
const deployConfig = await this.getUpdatedDeployConfig();
const deployTarget = await this.getDeployTarget(deployConfig);
const buildFilePaths = await this.getBuildFilePaths();
Comment on lines -194 to -196
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const deployId = await this.createNewDeploy(deployTarget);
private async cloudBuild(deployTarget: DeployTargetInfo) {
if (deployTarget.create) {
throw new Error("Incorrect deployTarget state");
}
const {deployPollInterval: pollInterval = DEPLOY_POLL_INTERVAL_MS} = this.deployOptions;
await this.apiClient.postProjectBuild(deployTarget.project.id);
const spinner = this.effects.clack.spinner();
spinner.start("Requesting deploy");
const pollExpiration = Date.now() + DEPLOY_POLL_MAX_MS;
while (true) {
if (Date.now() > pollExpiration) {
spinner.stop("Requesting deploy timed out.");
throw new CliError("Requesting deploy failed");
}
const {latestCreatedDeployId} = await this.apiClient.getProject({
workspaceLogin: deployTarget.workspace.login,
projectSlug: deployTarget.project.slug
});
if (latestCreatedDeployId !== deployTarget.project.latestCreatedDeployId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

spinner.stop(
`Deploy started. Watch logs: ${process.env["OBSERVABLE_ORIGIN"] ?? "https://observablehq.com"}/projects/@${
deployTarget.workspace.login
}/${deployTarget.project.slug}/deploys/${latestCreatedDeployId}`
);
// latestCreatedDeployId is initially null for a new project, but once
// it changes to a string it can never change back; since we know it has
// changed, we assert here that it’s not null
return latestCreatedDeployId!;
}
await new Promise((resolve) => setTimeout(resolve, pollInterval));
}
}

await this.uploadFiles(deployId, buildFilePaths);
await this.markDeployUploaded(deployId);
private async maybeLinkGitHub(deployTarget: DeployTargetInfo): Promise<boolean> {
if (deployTarget.create) {
throw new Error("Incorrect deployTarget state");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

}
if (!this.effects.isTty) return false;
Copy link
Contributor Author

@tophtucker tophtucker Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

if (deployTarget.environment.build_environment_id && deployTarget.environment.source) {
// can do cloud build
return true;
Comment on lines +244 to +246
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tophtucker tophtucker Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (isGit) {
const remotes = (await promisify(exec)("git remote -v", {cwd: this.deployOptions.config.root})).stdout
Copy link
Contributor Author

@tophtucker tophtucker Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (gitHub) {
const repoName = formatGitUrl(gitHub[1]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatGitUrl will also be different in the SSH case

const repositories = (await this.apiClient.getGitHubRepositories())?.repositories;
const authedRepo = repositories?.find(({url}) => formatGitUrl(url) === repoName);
Comment on lines +258 to +259
Copy link
Contributor Author

@tophtucker tophtucker Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels strange to ask for the list of all repositories instead of asking if you're authorized for a given repository! move this to API side and just ask github about the one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

).stdout;
await this.apiClient.postProjectEnvironment(deployTarget.project.id, {
source: {
provider: authedRepo.provider,
provider_id: authedRepo.provider_id,
url: authedRepo.url,
branch
}
});
return true;
} else {
// repo not auth’ed; link to auth page and poll for auth
// TODO: link to internal page that bookends the flow and handles the no-oauth-token case more gracefully
this.effects.clack.log.info(
`Authorize Observable to access the ${bold(repoName)} repository: ${link(
"https://github.com/apps/observable-data-apps-dev/installations/select_target"
)}`
);
const spinner = this.effects.clack.spinner();
spinner.start("Waiting for repository to be authorized");
const pollExpiration = Date.now() + DEPLOY_POLL_MAX_MS;
while (true) {
if (Date.now() > pollExpiration) {
spinner.stop("Waiting for repository to be authorized timed out.");
throw new CliError("Repository authorization failed");
}
const repositories = (await this.apiClient.getGitHubRepositories())?.repositories;
const authedRepo = repositories?.find(({url}) => formatGitUrl(url) === repoName);
if (authedRepo) {
spinner.stop("Repository authorized.");
await this.apiClient.postProjectEnvironment(deployTarget.project.id, {
source: {
provider: authedRepo.provider,
provider_id: authedRepo.provider_id,
url: authedRepo.url,
branch: null // TODO detect branch
}
});
return true;
}
await new Promise((resolve) => setTimeout(resolve, 2000));
}
}
} else {
this.effects.clack.log.error("No GitHub remote found; cannot enable continuous deployment.");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
} else {
this.effects.clack.log.error("Not at root of a git repository; cannot enable continuous deployment.");
}
}
return false;
}

private async startNewDeploy(): Promise<GetDeployResponse> {
const {deployConfig, deployTarget} = await this.getDeployTarget(await this.getUpdatedDeployConfig());
let deployId: string | null;
if (deployConfig.continuousDeployment) {
deployId = await this.cloudBuild(deployTarget);
} else {
const buildFilePaths = await this.getBuildFilePaths();
deployId = await this.createNewDeploy(deployTarget);
await this.uploadFiles(deployId, buildFilePaths);
await this.markDeployUploaded(deployId);
}
return await this.pollForProcessingCompletion(deployId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mythmon Eventually, yeah!

}

Expand Down Expand Up @@ -274,15 +401,18 @@ class Deployer {
}

// Get the deploy target, prompting the user as needed.
private async getDeployTarget(deployConfig: DeployConfig): Promise<DeployTargetInfo> {
private async getDeployTarget(
deployConfig: DeployConfig
): Promise<{deployTarget: DeployTargetInfo; deployConfig: DeployConfig}> {
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});
Copy link
Contributor Author

@tophtucker tophtucker Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

deployTarget = {create: false, workspace: project.owner, project, environment};
} catch (error) {
if (!isHttpError(error) || error.statusCode !== 404) {
throw error;
Expand Down Expand Up @@ -360,7 +490,17 @@ class Deployer {
workspaceId: deployTarget.workspace.id,
accessLevel: deployTarget.accessLevel
});
deployTarget = {create: false, workspace: deployTarget.workspace, project};
deployTarget = {
create: false,
workspace: deployTarget.workspace,
project,
// TODO: In the future we may have a default environment
environment: {
automatic_builds_enabled: null,
build_environment_id: null,
source: null
}
};
} catch (error) {
if (isApiError(error) && error.details.errors.some((e) => e.code === "TOO_MANY_PROJECTS")) {
this.effects.clack.log.error(
Expand All @@ -384,18 +524,40 @@ class Deployer {
}
}

let {continuousDeployment} = deployConfig;
if (continuousDeployment === null) {
const enable = await this.effects.clack.confirm({
message: wrapAnsi(
`Do you want to enable continuous deployment? ${faint(
"This builds in the cloud and redeploys whenever you push to this repository."
Comment on lines +531 to +532
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)}`,
this.effects.outputColumns
),
active: "Yes, enable and build in cloud",
inactive: "No, build locally"
});
if (this.effects.clack.isCancel(enable)) throw new CliError("User canceled deploy", {print: false, exitCode: 0});
continuousDeployment = enable;
}

// Disables continuous deployment if there’s no env/source & we can’t link GitHub
if (continuousDeployment) continuousDeployment = await this.maybeLinkGitHub(deployTarget);
Comment on lines +543 to +544
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const newDeployConfig = {
projectId: deployTarget.project.id,
projectSlug: deployTarget.project.slug,
workspaceLogin: deployTarget.workspace.login,
continuousDeployment
};

await this.effects.setDeployConfig(
this.deployOptions.config.root,
this.deployOptions.deployConfigPath,
{
projectId: deployTarget.project.id,
projectSlug: deployTarget.project.slug,
workspaceLogin: deployTarget.workspace.login
},
newDeployConfig,
this.effects
);

return deployTarget;
return {deployConfig: newDeployConfig, deployTarget};
}

// Create the new deploy on the server.
Expand Down Expand Up @@ -756,7 +918,17 @@ export async function promptDeployTarget(
if (effects.clack.isCancel(chosenProject)) {
throw new CliError("User canceled deploy.", {print: false, exitCode: 0});
} else if (chosenProject !== null) {
return {create: false, workspace, project: existingProjects.find((p) => p.slug === chosenProject)!};
// TODO(toph): initial env config
return {
create: false,
workspace,
project: existingProjects.find((p) => p.slug === chosenProject)!,
environment: {
automatic_builds_enabled: null,
build_environment_id: null,
source: null
}
};
}
} else {
const confirmChoice = await effects.clack.confirm({
Expand Down
61 changes: 61 additions & 0 deletions src/observableApiClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,35 @@ export class ObservableApiClient {
return await this._fetch<GetProjectResponse>(url, {method: "GET"});
}

async getProjectEnvironment({id}: {id: string}): Promise<GetProjectEnvironmentResponse> {
const url = new URL(`/cli/project/${id}/environment`, this._apiOrigin);
return await this._fetch<GetProjectEnvironmentResponse>(url, {method: "GET"});
}

async getGitHubRepositories(): Promise<GetGitHubRepositoriesResponse | null> {
const url = new URL("/cli/github/repositories", this._apiOrigin);
try {
return await this._fetch<GetGitHubRepositoriesResponse>(url, {method: "GET"});
} catch (err) {
return null;
}
}

async postProjectEnvironment(id, body): Promise<GetProjectEnvironmentResponse> {
const url = new URL(`/cli/project/${id}/environment`, this._apiOrigin);
return await this._fetch<GetProjectEnvironmentResponse>(url, {
method: "POST",
headers: {"Content-Type": "application/json"},
body: JSON.stringify(body)
});
}

async postProjectBuild(id): Promise<{id: string}> {
return await this._fetch<{id: string}>(new URL(`/cli/project/${id}/build`, this._apiOrigin), {
method: "POST"
});
}

async postProject({
title,
slug,
Expand Down Expand Up @@ -264,10 +293,42 @@ export interface GetProjectResponse {
title: string;
owner: {id: string; login: string};
creator: {id: string; login: string};
latestCreatedDeployId: string | null;
// Available fields that we don't use
// servingRoot: string | null;
}

export interface GetProjectEnvironmentResponse {
automatic_builds_enabled: boolean | null;
build_environment_id: string | null;
source: null | {
provider: string;
provider_id: string;
url: string;
branch: string | null;
};
}

export interface GetGitHubRepositoriesResponse {
installations: {
id: number;
login: string | null;
name: string | null;
}[];
repositories: {
provider: "github";
provider_id: string;
url: string;
default_branch: string;
name: string;
linked_projects: {
title: string;
owner_id: string;
owner_name: string;
}[];
}[];
}

export interface DeployInfo {
id: string;
status: string;
Expand Down
6 changes: 4 additions & 2 deletions src/observableApiConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export interface DeployConfig {
projectId?: string | null;
projectSlug: string | null;
workspaceLogin: string | null;
continuousDeployment: boolean | null;
}

export type ApiKey =
Expand Down Expand Up @@ -87,11 +88,12 @@ export async function getDeployConfig(
}
}
// normalize
let {projectId, projectSlug, workspaceLogin} = config ?? ({} as any);
let {projectId, projectSlug, workspaceLogin, continuousDeployment} = config ?? ({} as any);
if (typeof projectId !== "string") projectId = null;
if (typeof projectSlug !== "string") projectSlug = null;
if (typeof workspaceLogin !== "string") workspaceLogin = null;
return {projectId, projectSlug, workspaceLogin};
if (typeof continuousDeployment !== "boolean") continuousDeployment = null;
return {projectId, projectSlug, workspaceLogin, continuousDeployment};
}

export async function setDeployConfig(
Expand Down
Loading
Loading