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

[server] Two startPrebuild fixes #10154

Merged
merged 2 commits into from
May 20, 2022
Merged

[server] Two startPrebuild fixes #10154

merged 2 commits into from
May 20, 2022

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented May 20, 2022

Description

Comparing all call-sites of prebuildManager.startPrebuild yielded interesting insights:

(compare every call-site)

const projectAndOwner = await this.findProjectAndOwner(data.gitCloneUrl, user);
if (projectAndOwner.project) {
/* tslint:disable-next-line */
/** no await */ this.projectDB.updateProjectUsage(projectAndOwner.project.id, {
lastWebhookReceived: new Date().toISOString(),
});
}
const contextURL = this.createContextUrl(data);
const context = (await this.contextParser.handle({ span }, user, contextURL)) as CommitContext;
span.setTag("contextURL", contextURL);
const config = await this.prebuildManager.fetchConfig({ span }, user, context);
if (!this.prebuildManager.shouldPrebuild(config)) {
console.log("Bitbucket push event: No config. No prebuild.");
return undefined;
}
console.log("Starting prebuild.", { contextURL });
const { host, owner, repo } = RepoURL.parseRepoUrl(data.repoUrl)!;
const hostCtx = this.hostCtxProvider.get(host);
let commitInfo: CommitInfo | undefined;
if (hostCtx?.services?.repositoryProvider) {
commitInfo = await hostCtx.services.repositoryProvider.getCommitInfo(
user,
owner,
repo,
data.commitHash,
);
}
// todo@alex: add branch and project args
const ws = await this.prebuildManager.startPrebuild(
{ span },
{ user, project: projectAndOwner.project, context, commitInfo },
);

const projectAndOwner = await this.findProjectAndOwner(cloneUrl, user);
const config = await this.prebuildManager.fetchConfig({ span }, user, context);
if (!this.prebuildManager.shouldPrebuild(config)) {
console.log("Bitbucket push event: No config. No prebuild.");
return undefined;
}
console.debug("Bitbucket Server push event: Starting prebuild.", { contextUrl });
const commitInfo = await this.getCommitInfo(user, cloneUrl, commit);
const ws = await this.prebuildManager.startPrebuild(
{ span },
{
user: projectAndOwner.user,
project: projectAndOwner?.project,
context,
commitInfo,
},
);

let { user, project } = await this.findOwnerAndProject(installationId, cloneURL);
if (project) {
/* tslint:disable-next-line */
/** no await */ this.projectDB.updateProjectUsage(project.id, {
lastWebhookReceived: new Date().toISOString(),
});
}
const logCtx: LogContext = { userId: user.id };
if (!!user.blocked) {
log.info(logCtx, `Blocked user tried to start prebuild`, { repo: ctx.payload.repository });
return;
}
const pl = ctx.payload;
const branch = this.getBranchFromRef(pl.ref);
if (!branch) {
// This is a valid case if we receive a tag push, for instance.
log.debug("Unable to get branch from ref", { ref: pl.ref });
return;
}
const repo = pl.repository;
const contextURL = `${repo.html_url}/tree/${branch}`;
span.setTag("contextURL", contextURL);
const context = (await this.contextParser.handle({ span }, user, contextURL)) as CommitContext;
const config = await this.prebuildManager.fetchConfig({ span }, user, context);
const r = await this.ensureMainProjectAndUser(user, project, context, installationId);
user = r.user;
project = r.project;
const runPrebuild = this.appRules.shouldRunPrebuild(config, branch == repo.default_branch, false, false);
if (!runPrebuild) {
const reason = `Not running prebuild, the user did not enable it for this context`;
log.debug(logCtx, reason, { contextURL });
span.log({ "not-running": reason, config: config });
return;
}
const commitInfo = await this.getCommitInfo(user, repo.html_url, ctx.payload.after);
this.prebuildManager
.startPrebuild({ span }, { user, context, project, commitInfo })
.catch((err) => log.error(logCtx, "Error while starting prebuild", err, { contextURL }));

const projectAndOwner = await this.findProjectAndOwner(cloneURL, user);
if (projectAndOwner.project) {
/* tslint:disable-next-line */
/** no await */ this.projectDB.updateProjectUsage(projectAndOwner.project.id, {
lastWebhookReceived: new Date().toISOString(),
});
}
const contextURL = this.createContextUrl(payload);
span.setTag("contextURL", contextURL);
const context = (await this.contextParser.handle({ span }, user, contextURL)) as CommitContext;
const config = await this.prebuildManager.fetchConfig({ span }, user, context);
if (!this.prebuildManager.shouldPrebuild(config)) {
log.info("GitHub Enterprise push event: No config. No prebuild.");
return undefined;
}
log.debug("GitHub Enterprise push event: Starting prebuild.", { contextURL });
const commitInfo = await this.getCommitInfo(user, payload.repository.url, payload.after);
const ws = await this.prebuildManager.startPrebuild(
{ span },
{
context,
user: projectAndOwner.user,
project: projectAndOwner.project,
commitInfo,
},
);

const projectAndOwner = await this.findProjectAndOwner(context.repository.cloneUrl, user);
if (projectAndOwner.project) {
/* tslint:disable-next-line */
/** no await */ this.projectDB.updateProjectUsage(projectAndOwner.project.id, {
lastWebhookReceived: new Date().toISOString(),
});
}
const config = await this.prebuildManager.fetchConfig({ span }, user, context);
if (!this.prebuildManager.shouldPrebuild(config)) {
log.debug({ userId: user.id }, "GitLab push hook: There is no prebuild config.", {
context: body,
contextURL,
});
return undefined;
}
log.debug({ userId: user.id }, "GitLab push hook: Starting prebuild", { body, contextURL });
const commitInfo = await this.getCommitInfo(user, body.repository.git_http_url, body.after);
const ws = await this.prebuildManager.startPrebuild(
{ span },
{
user: projectAndOwner.user || user,
project: projectAndOwner.project,
context,
commitInfo,
},
);

  • The BitBucket Server integration does not update d_b_project_usage (fixed here)
  • The GitHub App happily triggers prebuilds for repositories that don't have prebuild tasks, or even a .gitpod.yml(!)

The second point is actually quite bad, because users may accidentally:

  • Install the GitHub App for all their repositories
  • Maintain some fully-automated repositories with high-frequency commits/pushes/PRs
  • This scenario can cause a very high volume of prebuilds, and for example produce our biggest GCP storage bucket (internal), among other things

Related Issue(s)

Fixes #

How to test

Release Notes

[server] Skip GitHub App prebuilds when the repository has no prebuild task(s) or .gitpod.yml

Documentation

@jankeromnes jankeromnes changed the title Two small startPrebuild fixes [server] Two small startPrebuild fixes May 20, 2022
@jankeromnes jankeromnes marked this pull request as ready for review May 20, 2022 13:36
@jankeromnes jankeromnes requested a review from a team May 20, 2022 13:36
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label May 20, 2022
@@ -102,6 +102,12 @@ export class BitbucketServerApp {
const cloneUrl = context.repository.cloneUrl;
const commit = context.revision;
const projectAndOwner = await this.findProjectAndOwner(cloneUrl, user);
if (projectAndOwner.project) {
/* tslint:disable-next-line */
/** no await */ this.projectDB.updateProjectUsage(projectAndOwner.project.id, {
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

@@ -102,6 +102,12 @@ export class BitbucketServerApp {
const cloneUrl = context.repository.cloneUrl;
const commit = context.revision;
const projectAndOwner = await this.findProjectAndOwner(cloneUrl, user);
if (projectAndOwner.project) {
/* tslint:disable-next-line */
/** no await */ this.projectDB.updateProjectUsage(projectAndOwner.project.id, {
Copy link
Contributor

Choose a reason for hiding this comment

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

why no await here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. This is how every other updateProjectUsage call site does it -- the thought was something like: This is not important right now, so it's fine to do it async without delaying the current prebuild/workspace start.

@roboquat roboquat merged commit 21d3f00 into main May 20, 2022
@roboquat roboquat deleted the jx/no-prebuild branch May 20, 2022 15:27
@jankeromnes jankeromnes changed the title [server] Two small startPrebuild fixes [server] Two startPrebuild fixes May 23, 2022
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants