Skip to content

[prebuild] Support opening a specfic prebuild #13706

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion components/dashboard/src/projects/Prebuild.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,9 @@ export default function () {
) : prebuild?.status === "available" ? (
<a
className="my-auto"
href={gitpodHostUrl.withContext(`${prebuild?.info.changeUrl}`).toString()}
href={gitpodHostUrl
.withContext(`open-prebuild/${prebuild?.info.id}/${prebuild?.info.changeUrl}`)
.toString()}
>
<button>New Workspace ({prebuild?.info.branch})</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the (branch) part of the button label no longer makes sense when we're opening the prebuild.

Maybe let's remove it?

Suggested change
<button>New Workspace ({prebuild?.info.branch})</button>
<button>New Workspace</button>

Or clarify it?

Suggested change
<button>New Workspace ({prebuild?.info.branch})</button>
<button>New Workspace With This Prebuild</button>

</a>
Expand Down
10 changes: 10 additions & 0 deletions components/gitpod-protocol/src/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,16 @@ export namespace AdditionalContentContext {
}
}

export interface OpenPrebuildContext extends WorkspaceContext {
openPrebuilID: string;
}

export namespace OpenPrebuildContext {
export function is(ctx: any): ctx is OpenPrebuildContext {
return "openPrebuildID" in ctx;
}
}

export interface CommitContext extends WorkspaceContext, GitCheckoutInfo {
/** @deprecated Moved to .repository.cloneUrl, left here for backwards-compatibility for old workspace contextes in the DB */
cloneUrl?: string;
Expand Down
19 changes: 15 additions & 4 deletions components/server/ee/src/workspace/gitpod-server-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
TeamMemberRole,
WORKSPACE_TIMEOUT_DEFAULT_SHORT,
PrebuildEvent,
OpenPrebuildContext,
} from "@gitpod/gitpod-protocol";
import { ResponseError } from "vscode-jsonrpc";
import {
Expand Down Expand Up @@ -963,9 +964,19 @@ export class GitpodServerEEImpl extends GitpodServerImpl {

const logCtx: LogContext = { userId: user.id };
const cloneUrl = context.repository.cloneUrl;
const prebuiltWorkspace = await this.workspaceDb
.trace(ctx)
.findPrebuiltWorkspaceByCommit(cloneUrl, commitSHAs);
let prebuiltWorkspace: PrebuiltWorkspace | undefined;
if (OpenPrebuildContext.is(context)) {
prebuiltWorkspace = await this.workspaceDb.trace(ctx).findPrebuildByID(context.openPrebuilID);
if (prebuiltWorkspace?.cloneURL !== cloneUrl) {
// prevent users from opening arbitrary prebuilds this way - they must match the clone URL so that the resource guards are correct.
return;
}
} else {
prebuiltWorkspace = await this.workspaceDb
.trace(ctx)
.findPrebuiltWorkspaceByCommit(cloneUrl, commitSHAs);
}

const logPayload = { mode, cloneUrl, commit: commitSHAs, prebuiltWorkspace };
log.debug(logCtx, "Looking for prebuilt workspace: ", logPayload);
if (!prebuiltWorkspace) {
Expand Down Expand Up @@ -994,7 +1005,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl {
const makeResult = (instanceID: string): WorkspaceCreationResult => {
return <WorkspaceCreationResult>{
runningWorkspacePrebuild: {
prebuildID: prebuiltWorkspace.id,
prebuildID: prebuiltWorkspace!.id,
workspaceID,
instanceID,
starting: "queued",
Expand Down
2 changes: 2 additions & 0 deletions components/server/src/container-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ import { LivenessController } from "./liveness/liveness-controller";
import { IDEServiceClient, IDEServiceDefinition } from "@gitpod/ide-service-api/lib/ide.pb";
import { prometheusClientMiddleware } from "@gitpod/gitpod-protocol/lib/util/nice-grpc";
import { UsageService } from "./user/usage-service";
import { OpenPrebuildPrefixContextParser } from "./workspace/open-prebuild-prefix-context-parser";

export const productionContainerModule = new ContainerModule((bind, unbind, isBound, rebind) => {
bind(Config).toConstantValue(ConfigFile.fromFile());
Expand Down Expand Up @@ -189,6 +190,7 @@ export const productionContainerModule = new ContainerModule((bind, unbind, isBo
bind(IPrefixContextParser).to(EnvvarPrefixParser).inSingletonScope();
bind(IPrefixContextParser).to(ImageBuildPrefixContextParser).inSingletonScope();
bind(IPrefixContextParser).to(AdditionalContentPrefixContextParser).inSingletonScope();
bind(IPrefixContextParser).to(OpenPrebuildPrefixContextParser).inSingletonScope();

bind(GitTokenScopeGuesser).toSelf().inSingletonScope();
bind(GitTokenValidator).toSelf().inSingletonScope();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* Copyright (c) 2021 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License-AGPL.txt in the project root for license information.
*/

import { User, WorkspaceContext } from "@gitpod/gitpod-protocol";
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
import { OpenPrebuildContext } from "@gitpod/gitpod-protocol/src/protocol";
import { inject, injectable } from "inversify";
import { Config } from "../config";
import { IPrefixContextParser } from "./context-parser";

@injectable()
export class OpenPrebuildPrefixContextParser implements IPrefixContextParser {
@inject(Config) protected readonly config: Config;
static PREFIX = /^\/?open-prebuild\/([^\/]*)\//;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, many thanks @csweichel for implementing a new context URL prefix for this!

However, I'm curious why a new prefix is more beneficial than "just" pointing a Prebuild view's "New Workspace" button to the (already-supported) fully-qualified commit context URL -- if you open the exact commit that was used to create the prebuild, you're guaranteed to get this prebuild, right? 💭

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 you open the exact commit that was used to create the prebuild, you're guaranteed to get this prebuild, right?

Aye. Two reasons not to build it this way:

  1. I did not see a clean way to construct the commit context URL for all SCM provider
  2. It relies on the behaviour that we do actually open the prebuild for that commit given the correct context URL. Given the reliability of this feature I wanted to make this rock solid and identify the prebuild directly.

Another question would be: why use a prefix context parser rather than a regular context parser. This also has a couple of reasons:

  • regular context parser are tied to the SCM system, i.e. assume they're part of the SCM inversify container. A PrebuildContextParser would break this assumption.
  • permission checks can only happen in the server - if we wanted this to be a context parser, we'd need to make the permission checks part of that flow. Using a prefix context parser we can just rely on the permission checks already present in the SCM context parsing flow. This is an indicator to me that the current authorisation model has deficits we need to fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Makes sense. (I guess to achieve the commit context URL trick, we'd need to always store the commit's treeURL in our metadata. But I'm happy to go with your approach that identifies prebuilds directly. 👍)


findPrefix(user: User, context: string): string | undefined {
const result = OpenPrebuildPrefixContextParser.PREFIX.exec(context);
if (!result) {
return undefined;
}
return result[0];
}

public async handle(user: User, prefix: string, context: WorkspaceContext): Promise<WorkspaceContext> {
const match = OpenPrebuildPrefixContextParser.PREFIX.exec(prefix);
if (!match) {
log.error("Could not parse prefix " + prefix);
return context;
}

(context as OpenPrebuildContext).openPrebuilID = match[1];
return context;
}
}