-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[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
Conversation
started the job as gitpod-build-cw-open-prebuild.3 because the annotations in the pull request description changed |
22044b9
to
5ccb4b1
Compare
/werft run 👍 started the job as gitpod-build-cw-open-prebuild.5 |
started the job as gitpod-build-cw-open-prebuild.6 because the annotations in the pull request description changed |
/werft run 👍 started the job as gitpod-build-cw-open-prebuild.7 |
@injectable() | ||
export class OpenPrebuildPrefixContextParser implements IPrefixContextParser { | ||
@inject(Config) protected readonly config: Config; | ||
static PREFIX = /^\/?open-prebuild\/([^\/]*)\//; |
There was a problem hiding this comment.
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? 💭
There was a problem hiding this comment.
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:
- I did not see a clean way to construct the commit context URL for all SCM provider
- 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.
There was a problem hiding this comment.
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. 👍)
Note: I'll give this PR a quick rebase, in the hopes of getting it to build. 🤞 |
5ccb4b1
to
a2712f0
Compare
Insert coin to try again 🎰 /werft run 👍 started the job as gitpod-build-cw-open-prebuild.9 |
And again /werft run 👍 started the job as gitpod-build-cw-open-prebuild.10 |
@jankeromnes this might need a clean slate 🍽️ 😄 |
Indeed: /werft run with-clean-slate-deployment 👍 started the job as gitpod-build-cw-open-prebuild.11 |
I guess that luck is permanently stuck. Maybe let's move this work to a different branch. Happy to do this now. 🚚 |
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> |
There was a problem hiding this comment.
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?
<button>New Workspace ({prebuild?.info.branch})</button> | |
<button>New Workspace</button> |
Or clarify it?
<button>New Workspace ({prebuild?.info.branch})</button> | |
<button>New Workspace With This Prebuild</button> |
Okay, I've tested this (over in #13768), but it doesn't see to do what I expected:
But, clicking on "New Workspace" in the prebuild of "commit A" should give me a workspace based on "commit A", right? |
Let's move the conversation over to #13768. Will pick it up from there. |
Description
This PR enables users to open a specific prebuildm, and integrates this capability on the dashboard.
How to test
Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide