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

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
adaa867
checkpoint - kicking off cloud build from cli sorta working
tophtucker Oct 4, 2024
fa5e762
correct ellipsis
tophtucker Oct 8, 2024
6e614ca
continuousDeployment flag persisted in deploy.json
tophtucker Oct 11, 2024
a7efd4c
poll for repo access
tophtucker Oct 11, 2024
1077838
cleanup. remove "Do you wanna link GitHub" prompt, since you have to
tophtucker Oct 11, 2024
44b1e5f
various post-demo cleanup; maybeLinkGitHub even if we’re not enabling…
tophtucker Oct 15, 2024
b6c2a3f
better handling of the case where github is not connected at all; set…
tophtucker Oct 15, 2024
bb790ef
throw Error → throw new Error
tophtucker Oct 15, 2024
93203ca
Merge branch 'main' into toph/onramp
tophtucker Oct 15, 2024
8128173
fix existing tests
tophtucker Oct 15, 2024
56e4d14
start on docs
tophtucker Oct 22, 2024
6f7c852
Merge branch 'main' into toph/onramp
tophtucker Oct 25, 2024
9e234a2
use new singular endpoint to see if repo is authed
tophtucker Oct 25, 2024
c9effe0
clean up logic, more dry
tophtucker Oct 26, 2024
8c75f52
rm getProjectEnvironment calls
tophtucker Oct 29, 2024
75cbf50
link to internal interstitial screen instead of directly to github
tophtucker Oct 30, 2024
e80786b
slightly more human-readable error message
tophtucker Oct 30, 2024
ea676b1
dont change continuousDeployment setting as a side effect of github l…
tophtucker Oct 30, 2024
06b52f7
flatten structure of maybeLinkGitHub with early returns
tophtucker Oct 31, 2024
a7ba250
rename maybeLinkGitHub: boolean to validateGitHubLink: void, more err…
tophtucker Oct 31, 2024
5830afc
support SSH github remotes
tophtucker Nov 1, 2024
048fd88
move validateGitHubLink call to a better higher clearer more consolid…
tophtucker Nov 1, 2024
7c719ae
tweak cd prompt to clarify that you need a github repo
tophtucker Nov 1, 2024
a6ac45e
Merge branch 'main' into toph/onramp
tophtucker Nov 1, 2024
8407f88
minimize diff
tophtucker Nov 1, 2024
476ebec
minimize diff, again
tophtucker Nov 1, 2024
bc60124
first tests of validateGitHubLink
tophtucker Nov 2, 2024
14ce19b
whoohoo end-to-end test of kicking off cloud build
tophtucker Nov 2, 2024
27128bb
ALL TESTS PASSING incl coverage threshold, thanks to testing cloud bu…
tophtucker Nov 2, 2024
3a5de29
Merge branch 'main' into toph/onramp
tophtucker Nov 2, 2024
82f7b0d
move mockIsolatedDirectory to own file
tophtucker Nov 2, 2024
105f0ed
Merge branch 'main' into toph/onramp
tophtucker Nov 2, 2024
e511cc7
testing if git is installed on ubuntu
tophtucker Nov 2, 2024
bb9c311
testing deterministic default branch name
tophtucker Nov 2, 2024
ff60b80
more debugging...
tophtucker Nov 2, 2024
f1cd74d
setting name and email config for git, seeing if that helps
tophtucker Nov 2, 2024
e188a39
fix command separator from ; to && for windows; prettier for ubuntu
tophtucker Nov 2, 2024
fae07e1
use fs instead of touch for cross-platform (windows) compatibility; f…
tophtucker Nov 2, 2024
41f9682
force: true on rm dir for windows complaining ENOTEMPTY
tophtucker Nov 2, 2024
dd50d70
adopting rimraf in lieu of node fs rm for what i hope is better windo…
Nov 2, 2024
3f0bbb8
oops i wasnt actually closing the file i made. good call, windows. ro…
Nov 2, 2024
fede926
ah i had another case of touch
Nov 2, 2024
172bf50
new test for when repo doesnt match
Nov 2, 2024
87f707d
add test for polling for repo auth; deploy.ts test coverage is now ba…
Nov 2, 2024
159d708
WHEW i think everything should be passing now, removing debug console…
Nov 2, 2024
1a5c9f9
lmao last few commits are attributed to my test user bc the exec git …
tophtucker Nov 2, 2024
4a8e4e7
now that were not setting the git config options globally we have to …
Nov 2, 2024
a23c35c
set initial branch when initializing repo instead of setting the defa…
Nov 2, 2024
36e2153
Merge branch 'main' into toph/onramp
Fil Nov 8, 2024
3e5e669
Fil/onramp review (#1805)
Fil Nov 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 188 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,124 @@ 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const deployId = await this.createNewDeploy(deployTarget);
private async cloudBuild(deployTarget: DeployTargetInfo) {
if (deployTarget.create) {
throw Error("Incorrect deployTarget state");
tophtucker marked this conversation as resolved.
Show resolved Hide resolved
}
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not planning to do anything about this for now

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 Error("Incorrect deployTarget state");
tophtucker marked this conversation as resolved.
Show resolved Hide resolved
}
if (!this.effects.isTty) return false;
tophtucker marked this conversation as resolved.
Show resolved Hide resolved
if (deployTarget.environment.build_environment_id && deployTarget.environment.source) {
// can do cloud build
return true;
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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tophtucker tophtucker Oct 31, 2024

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const {repositories} = await this.apiClient.getGitHubRepositories();
const authedRepo = repositories.find(({url}) => formatGitUrl(url) === repoName);
if (authedRepo) {
// authed repo found
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;
} else {
// repo not auth’ed; link to auth page and poll for auth
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("Deploy failed");
}
const {repositories} = await this.apiClient.getGitHubRepositories();
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");
}
} else {
this.effects.clack.log.error("Not at root of a git repository");
}
}
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);
tophtucker marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -274,15 +397,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});
tophtucker marked this conversation as resolved.
Show resolved Hide resolved
deployTarget = {create: false, workspace: project.owner, project, environment};
} catch (error) {
if (!isHttpError(error) || error.statusCode !== 404) {
throw error;
Expand Down Expand Up @@ -360,7 +486,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 +520,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."
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

)}`,
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);
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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 +914,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
57 changes: 57 additions & 0 deletions src/observableApiClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,31 @@ 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> {
const url = new URL("/cli/github/repositories", this._apiOrigin);
return await this._fetch<GetGitHubRepositoriesResponse>(url, {method: "GET"});
}

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 +289,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