Link prebuilds with projects #4727
Conversation
AlexTugarev
force-pushed
the
at/select-repo
branch
from
f776473
to
ef36f16
Compare
AlexTugarev
force-pushed
the
at/select-repo
branch
2 times, most recently
from
695d6bb
to
93195a4
Compare
|
/werft run 👍 started the job as gitpod-build-at-select-repo.84 |
There was a problem hiding this comment.
Many thanks @AlexTugarev! The code looks good to me.
How should this change be tested? (In particular, prebuilds in relation to the GitHub/GitLab/Bitbucket apps/integrations.) I've briefly tested that the /#prebuild/ manual prefix still works, but I'd love to play with a proper App/Integration in order to confirm that everything (still) works as expected.
| // | ||
| const project = await this.projectDB.findProjectByInstallationId(String(installationId)); | ||
| if (project) { | ||
| const owner = (await this.teamDB.findMembersByTeam(project.teamId)).filter(m => m.role === "owner")[0]; |
There was a problem hiding this comment.
When there are multiple owners in a team, here we pick a "random" owner. Is this okay/expected?
It could very well be a different user than whoever installed the app for a given project in that team.
There was a problem hiding this comment.
It could very well be a different user than whoever installed the app for a given project in that team.
This is a fair point. We haven't discussed the differentiation between installer and owner. I guess for now it's good enough, as there is no accounting for this.
|
|
||
| // Legacy mode | ||
| // | ||
| const installation = await this.appInstallationDB.findInstallation("github", String(installationId)); |
There was a problem hiding this comment.
When we do find a project + owner + user above, we no longer perform this check at all.
Do we really want to trust projectDB and teamDB to implicitly guarantee the existence / validity of a given installationId?
(E.g. what happens if I install the Gitpod app to add some project to some team, but then uninstall the app again? Or it gets revoked? Or I leave the team?)
I would feel a bit safer if this sanity check is common to both modes ("project" and "legacy").
There was a problem hiding this comment.
Do we really want to trust projectDB and teamDB to implicitly guarantee the existence / validity of a given installationId?
you are absolutely right about the "uninstalled" event. It should also either clean up or disable projects.
putting this on TODO list.
what else would you expect?
There was a problem hiding this comment.
what else would you expect?
For the sanity check "does this installation ID actually correspond to anything?" to be above both "modes" (not just performed in "legacy" mode as currently). You can simply move the entire const installation = ... and if (!installation) block up several lines.
| const res = await query.getMany(); | ||
| return res; |
There was a problem hiding this comment.
| const res = await query.getMany(); | |
| return res; | |
| return query.getMany(); |
| const inst = await this.internalStoreInstance(instance); | ||
| return inst; |
There was a problem hiding this comment.
| const inst = await this.internalStoreInstance(instance); | |
| return inst; | |
| return this.internalStoreInstance(instance); |
| @@ -47,6 +46,8 @@ export class WorkspaceFactoryEE extends WorkspaceFactory { | |||
| throw new Error("Can only prebuild workspaces with a commit context") | |||
| } | |||
|
|
|||
| const { project, branch } = context; | |||
There was a problem hiding this comment.
Do you expect to use project or branch somewhere else in the code below?
Otherwise, you could avoid the single-use local variables by using context.project and context.branch directly in the storePrebuiltWorkspace() call below.
There was a problem hiding this comment.
After testing this some more, with an installed GitHub App, I'm more confident that this works as expected. Thus, approving. Thanks again!
However, could you please wait and only merge this after tomorrow's production deployment? This would allow more time to test on staging as well.
EDIT: Also, please squash all related commits together.
@jankeromnes, thank you very much! I'll do so. |
AlexTugarev
force-pushed
the
at/select-repo
branch
from
056e16f
to
b37fddd
Compare
AlexTugarev
force-pushed
the
at/select-repo
branch
from
b37fddd
to
51a524e
Compare
With this PR push events will trigger prebuilds and associate them with a project (
PrebuiltWorkspace.projectId) when the App installation is found.This also includes a deduplication fix for the drop down on New Project: