-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[prebuilds] no prebuilds for inactive repos #9976
Conversation
dc3adac
to
cc322ec
Compare
cc322ec
to
a4a5737
Compare
/werft run 👍 started the job as gitpod-build-se-no-prebuilds-inactive.3 |
fa5fe1a
to
969a0a8
Compare
/werft run with-clean-slate-deployment=true 👍 started the job as gitpod-build-se-no-prebuilds-inactive.9 |
components/gitpod-db/src/typeorm/migration/1652365883273-CloneUrlIndexed.ts
Outdated
Show resolved
Hide resolved
components/gitpod-db/src/typeorm/migration/1652365883273-CloneUrlIndexed.ts
Outdated
Show resolved
Hide resolved
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.
969a0a8
to
17539d9
Compare
84f5634
to
ce6b918
Compare
No longer using the GENERATED COLUMN, thus releasing the break.
I've added ce6b918 to decouple deployment from enablement. That means, we can migrate as much entries as needed and then reconfigure server to enable this feature. |
Thanks for the review request! And for adding the config flag. I'm a bit worried now, because this Pull Request is growing more and more in complexity. At first it was a quick fix to disable prebuilds for inactive repo URLs, but now we have:
This is starting to look a bit less like a skateboard. 😕 How about, instead, we purely and simply disable all prebuilds that do not have a project? That's something we want to do anyway soon, and it resolves the same problem in a very simple, risk-free way. |
It doesn't add complexity as it is well encapsulated in a single function and the max length we had in the past 3 months is 159.
Why is that migration problematic or taking a long time? It's adding an empty column.
I agree that is a bit over the top as this was intended an intermediate fix, but really no reason to not move this forward.
There are way too many important active projects to "simply" do this without notice. Can we please get this in now and move on? |
There is no need to be flexible about the activity time. |
Okay, giving this a closer look and a vigorous shake (i.e. testing it). |
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.
Code is a bit more complicated than I'd like but still looks correct. Also, I tested this a lot, and it still works as advertised.
It's nice to be able to disable this by setting inactivityPeriodForRepos
to 0
in the server configmap (also tested this, works).
So, approving, but I'm very much looking forward to us simply disabling prebuilds for non-projects (and deleting the /#prebuild/
manual prefix and all of its problems).
Thanks again! Ad astra 🚀
default: "", | ||
transformer: Transformer.MAP_EMPTY_STR_TO_UNDEFINED, | ||
}) | ||
cloneUrl?: string; |
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 this can never be undefined
right?
cloneUrl?: string; | |
cloneUrl: string; |
@@ -0,0 +1,28 @@ | |||
/** | |||
* Copyright (c) 2021 Gitpod GmbH. All rights reserved. |
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.
Micro-nit:
* Copyright (c) 2021 Gitpod GmbH. All rights reserved. | |
* Copyright (c) 2022 Gitpod GmbH. All rights reserved. |
Description
This change prevents prebuilds for repositories that have been inactive (no regular workspace starts) for at least a week.
This would have prevented 98835 unnecessary prebuilds in the last 11 days (vs. 28174 prebuilds on repos without projects that are still active).
Related Issue(s)
See also my comment here #9898 (comment)
Fixes #9962
How to test
(AT) Unfortunately this isn't easy to test, because this requires using the old-style
/prebuild#
triggers and DB connection to verify prebuild statuses. Here is the SQL output of my test run, injecting the following markers:"inactivityPeriodForRepos": 7
toserver-config
/prebuild#
trigger instead of having a projectRelease Notes