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

feat(server): Server should periodically delete old builds #471

Merged
merged 2 commits into from Oct 18, 2020

Conversation

koh110
Copy link
Contributor

@koh110 koh110 commented Oct 15, 2020

I add cron task that delete old builds on sql to the server.

fixes: #463

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

thank you @koh110 this is fantastic!! 🎉

those tests and refactor to share cron utils are great 💯

* @param {Date} runAt
* @return {Promise<LHCI.ServerCommand.Build[]>}
*/
async findOldBuilds(runAt) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

findBuildsBeforeTimestamp maybe?

throw new Error('Invalid range');
}

const runAt = new Date(now.setDate(now.getDate() + range));
Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about...

Suggested change
const runAt = new Date(now.setDate(now.getDate() + range));
const DAY_IN_MS = 24 * 60 * 60 * 1000;
const cutoffTime = new Date(Date.now() - maxAgeInDays * DAY_IN_MS));

or something like that? setDate is clever but I worry it relies on a bit too much knowledge of range needing to be negative in config, a bit harder to think through for correctness across month/year boundaries, etc

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 agree with your comment.

I worry it relies on a bit too much knowledge of range needing to be negative in config

Your code looks to be easier than my code to understand. Thanks for the good suggestions.

I will change it.

return;
}

if (!options.storage.deleteOldBuilds.schedule || !options.storage.deleteOldBuilds.dateRange) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of dateRange what do you think about maxAgeInDays

@@ -173,6 +173,10 @@ declare global {
sqlConnectionUrl?: string;
sqlDangerouslyResetDatabase?: boolean;
sequelizeOptions?: import('sequelize').Options;
deleteOldBuilds?: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this belongs at the StorageOptions layer since the storage class doesn't know about or do anything with this cron at all.

can we move up to Options?

WDYT about deleteOldBuildsCron

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function only works when the storage option is 'sql', so I put it in StorageOption.
But, I agree with your comment. I move up to Options.

@@ -44,7 +44,6 @@ declare global {

export interface Options {
url?: string | string[];
autodiscoverUrlBlocklist?: string | string[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert this please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my mistake! I overlooked that. Thanks.

@@ -0,0 +1,21 @@
'use strict';
Copy link
Collaborator

Choose a reason for hiding this comment

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

add license comment

packages/server/src/cron/utils.js Show resolved Hide resolved
@@ -55,6 +56,7 @@ async function createApp(options) {
app.use(errorMiddleware);

startPsiCollectCron(storageMethod, options);
startDeletingOldBuildsCron(storageMethod, options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

for consistency with option proposed name startDeleteOldBuildsCron?

packages/server/src/cron/delete-old-builds.js Show resolved Hide resolved
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM thank you @koh110! 🎉

@patrickhulce patrickhulce merged commit c324f3f into GoogleChrome:master Oct 18, 2020
2 checks passed
@koh110
Copy link
Contributor Author

koh110 commented Oct 19, 2020

@patrickhulce Thank you for your review & merge!
I can't wait to use this feature in my lighthouse-ci! Is there anything I can do to help with the release?

@patrickhulce
Copy link
Collaborator

Nothing in particular, I'll try to ship it out this week 👍

@koh110
Copy link
Contributor Author

koh110 commented Oct 19, 2020

I appreciate you! 👍

denkrasnov pushed a commit to Travix-International/lighthouse-ci that referenced this pull request Oct 29, 2020
@emmekappa
Copy link

@patrickhulce any idea when this will be released? Is it planned for the 0.6.2?

@patrickhulce
Copy link
Collaborator

@emmekappa this has been available since v0.5.1

@emmekappa
Copy link

@patrickhulce is somehow possible to configure the numbers of days? I cannot see anything in the doc

@patrickhulce
Copy link
Collaborator

@emmekappa yes it is, it accepts two options. maxAgeInDays is what you're looking for

It wasn't added to the docs yet, but PR welcome :)

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

Successfully merging this pull request may close these issues.

Server should periodically delete old builds
3 participants