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.

Sign up for GitHub

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

Jump to bottom

[prebuild] Support opening a specfic prebuild #13768

Merged
merged 1 commit into from
Oct 14, 2022
Merged

[prebuild] Support opening a specfic prebuild #13768

merged 1 commit into from
Oct 14, 2022

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Oct 11, 2022 •
edited by jldec
Loading

(Werft was having too much trouble with #13706 hence the new PR 👇)

Description

This PR enables users to open a specific prebuildm, and integrates this capability on the dashboard.

Fixes #13900

How to test

  1. Make sure there's a prebuild and a newer commit on the same branch
  2. Go the prebuild view
  3. Open the specific prebuild and observe it's the correct state

Release Notes

[prebuild] Add support for opening specific prebuilds

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

Front logo Front conversations

@jankeromnes jankeromnes mentioned this pull request Oct 11, 2022
Closed
3 tasks
@csweichel csweichel marked this pull request as ready for review October 13, 2022 09:35
@csweichel csweichel requested a review from a team October 13, 2022 09:35
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 13, 2022
@csweichel
Copy link
Contributor

csweichel commented Oct 13, 2022 •
edited
Loading

Change works now as intended. I've also changed the wording on the button.

I am not particularly fond of the hoops I had to introduce to make the overall change happen!

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Oct 13, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Awesome news, many thanks @csweichel! 🎉

The Werft build failed with:

  1) AccountServiceSpec
       "before each" hook for "multiCreditSessionAfter":
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/tmp/build/components-ee-payment-endpoint--dbtest.8bbb983371d02e98c8c1e2bb25cf11d71d9a6e35/src/accounting/account-service.spec.db.ts)

we could raise this timeout to 30s. But meanwhile:

/werft run

👍 started the job as gitpod-build-cw-open-prebuild-2.2
(with .werft/ from main)

@geropl
Copy link
Member

geropl commented Oct 13, 2022

I am not particularly fond of the hoops I had to introduce to make the overall change happen!

@csweichel While I see that this is a minimal change to enable opening workspaces arbitrary prebuilds: Is this really something we want to enable using a ContextParser? That somehow feels like a hack to me (and might cause some of the "hoops" you were referring to).

I'm ok with this if we want to have something to move forward and test quickly. But I'd rather not ask customers to use these ContextUrls. 😕 I'd rather go with something akin Jan is looking into and call that from the dashboard in appropriate places. WDYT?

@csweichel
Copy link
Contributor

csweichel commented Oct 13, 2022 •
edited
Loading

Is this really something we want to enable using a ContextParser? That somehow feels like a hack to me (and might cause some of the "hoops" you were referring to).

Ideally I'd like to have an actual proper context for opening "named" prebuilds. Unfortunately ContextParser are tied to SCM systems and are hard to use outside of that context. This causes hoop 1: using a prefix context parser.

The way we currently integrate prebuilds in the workspace creation and context handling is a bit of a "slight of hand" style thing with a few implicit assumptions/behaviours sprinkled in, which e.g. enable the incremental nature of things today. That's what necessitates hoop 2: the copying of the original context and removal of ref, because we ignore the refType. I didn't want to enforce the refType for fear of adverse side effects.

Testing all this stuff is near impossible. I tried to introduce a unit test for findPrebuildWorkspace but gave up going down the inversify rabbit hole 30 minutes in. There are few (no?) tests around workspace creation which makes introducing changes safely difficult. That's hoop 3: I was unable to tie this behaviour down, and only through stepping through the flow was I able to understand what's even happening.

How do we ensure that users actually get the prebuild they selected, if not through context? Relying on the commit matching alone feels too brittle.

@geropl
Copy link
Member

geropl commented Oct 13, 2022

Ideally I'd like to have an actual proper context for opening "named" prebuilds

I agree here if by "named" you mean precisely define which prebuild you want. 👍

How do we ensure that users actually get the prebuild they selected, if not through context?

I thought about this as more of a parameter to "createWorkspace" which controls where to initialize content from, rather than a Context. Content depends on the Context, but they are not the same.

Testing all this stuff is near impossible.

There are few (no?) tests around workspace creation which makes introducing changes safely difficult.

Absolutely agree. That's one area where we never found time yet to improve upon.

geropl
geropl reviewed Oct 14, 2022
View reviewed changes
components/server/ee/src/workspace/workspace-factory.ts
@@ -373,6 +374,18 @@ export class WorkspaceFactoryEE extends WorkspaceFactory {
config._featureFlags = (config._featureFlags || []).concat(["persistent_volume_claim"]);
}

if (OpenPrebuildContext.is(context.originalContext)) {
// Because of incremental prebuilds, createForContext will take over the original context.
Copy link
Member

Choose a reason for hiding this comment

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

👍

geropl
geropl reviewed Oct 14, 2022
View reviewed changes
components/gitpod-protocol/src/protocol.ts
@@ -1200,6 +1200,16 @@ export namespace AdditionalContentContext {
}
}

export interface OpenPrebuildContext extends WorkspaceContext {
Copy link
Member

Choose a reason for hiding this comment

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

While I'm not super-fond of adding a ContextParser for this I understand that currently, it's the only accessible way that make things like this work.
Also, removing it later should not be too hard. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we use the commit context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See related discussion: #13706 (comment)

geropl
geropl approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested and works. ✨

@geropl geropl force-pushed the cw/open-prebuild-2 branch from a8bb384 to 8b4b8dc Compare October 14, 2022 14:20
@geropl
Copy link
Member

geropl commented Oct 14, 2022

rebased to make the build green... 🤞

@roboquat roboquat merged commit 8155f24 into main Oct 14, 2022
@roboquat roboquat deleted the cw/open-prebuild-2 branch October 14, 2022 14:33
@jldec jldec mentioned this pull request Oct 17, 2022
Closed
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 17, 2022
@jankeromnes jankeromnes mentioned this pull request Oct 18, 2022
Merged
3 tasks
This was referenced Oct 20, 2022
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svenefftinge svenefftinge svenefftinge left review comments

@geropl geropl geropl approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow workspaces to start from prebuilds built against older git commits
5 participants
@jankeromnes @csweichel @geropl @svenefftinge @roboquat