Skip to content

Enhance heuristics to delete preview-environments #9225

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

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Conversation

fullmetalrooster
Copy link
Contributor

@fullmetalrooster fullmetalrooster commented Apr 11, 2022

Description

This improves the detection of outdated preview-environments. The database of each instance is tested if the last access is more than 24h ago based on the data provided by d_b_workspace_instance.creationTime, d_b_user.creationDate, d_b_user._lastModified and d_b_workspace_instance_user.lastSeen. It also checks if the last commit of a branch is less than 5 days ago and deletes the preview-environment if there has been no change.

Related Issue(s)

Fixes https://github.com/gitpod-io/ops/issues/1623

How to test

Open a new workspace with this PR and run werft run github -j .werft/platform-delete-preview-environments-cron.yaml -f. Preview environments are not deleted as the necessary function needs to be uncommented.

Release Notes

None

Documentation

@mads-hartmann
Copy link
Contributor

mads-hartmann commented Apr 19, 2022

@wulfthimm I started reviewing this but won't finish until tomorrow I'm afraid ☺️

@mads-hartmann mads-hartmann self-requested a review April 19, 2022 15:04
@fullmetalrooster
Copy link
Contributor Author

@mads-hartmann No problem. I could also walk you trough and maybe learn something on the way as I am no TS developer.

Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

Thanks! I left a few comments around changes I'd like to see ☺️

@fullmetalrooster
Copy link
Contributor Author

@mads-hartmann Thanks for the comments. I really appreciate the insights. I will address them once I finished my current work. I expect to have them fixed over the next day.

@roboquat roboquat added size/L and removed size/M labels Apr 21, 2022
@fullmetalrooster fullmetalrooster force-pushed the wth/impr-clean-up branch 3 times, most recently from 6a8283a to b7dc0ed Compare April 22, 2022 09:04
Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

Thanks Wulf! I have another round of small change requests and then I think we're done ☺️

@fullmetalrooster
Copy link
Contributor Author

@mads-hartmann Thanks for your comments. I will make the changes on monday.

@fullmetalrooster
Copy link
Contributor Author

@mads-hartmann I made the requested changes and I hope it is correct now.

werft.log("deleting preview", preview)
promises.push(wipePreviewEnvironmentAndNamespace(helmInstallName, preview, CORE_DEV_KUBECONFIG_PATH, { slice: `Deleting preview ${preview}` }))
})
await Promise.all(promises)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change the mapping as the compiler throw an arrow that .map is not supported on this set.

@@ -6,6 +6,10 @@ import { exec } from './util/shell';
import { previewNameFromBranchName } from './util/preview';
import { CORE_DEV_KUBECONFIG_PATH } from './jobs/build/const';

// for testing purposes
// if set to 'true' it shows only previews that would be deleted
const DRY_RUN = true
Copy link
Contributor Author

@fullmetalrooster fullmetalrooster Apr 25, 2022

Choose a reason for hiding this comment

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

I keep the hold label and change it to false once this PR is approved.

@fullmetalrooster fullmetalrooster force-pushed the wth/impr-clean-up branch 2 times, most recently from aac45ed to 9367df4 Compare April 25, 2022 13:21
@mads-hartmann
Copy link
Contributor

mads-hartmann commented Apr 26, 2022

@wulfthimm I just used this job to clean up 94 preview environments 🎉 The Werft build is failing though. Perhaps trigger a new build, maybe after rebasing on main just to be on the safe side ☺️ Please update DRY_RUN to false too and then I'll approve ☺️

@fullmetalrooster fullmetalrooster force-pushed the wth/impr-clean-up branch 2 times, most recently from 23b529b to 181daed Compare April 26, 2022 09:56
Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! 🛹 Remove the hold label and merge whenever you're comfortable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants