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.

Sign up for GitHub

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

Updated croner to 8.1.0 and fixed "recurring-interval" type maintenance #4939

Merged
merged 8 commits into from
Jul 15, 2024
Merged

Updated croner to 8.1.0 and fixed "recurring-interval" type maintenance #4939

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d616883
Updated croner to 8.1.0 and fixed "recurring-interval" type maintenance
buzzinJohnnyBoi Jul 13, 2024
4750bd1
Fixed "recurring-interval" type maintenance - added comments
buzzinJohnnyBoi Jul 13, 2024
7f64bad
Update comment on recurring-interval type maintenance
buzzinJohnnyBoi Jul 14, 2024
4836f72
added inferDuration to calculate cron duration
buzzinJohnnyBoi Jul 15, 2024
30f7b48
updated comments and ran linter on inferDuration
buzzinJohnnyBoi Jul 15, 2024
76deb6e
package.json - Update dependency version range for croner
buzzinJohnnyBoi Jul 15, 2024
e791c70
Fixed the package-lock.json
CommanderStorm Jul 15, 2024
1822b55
Merge branch 'master' into master
CommanderStorm Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
"command-exists": "~1.2.9",
"compare-versions": "~3.6.0",
"compression": "~1.7.4",
"croner": "~6.0.5",
"croner": "^8.1.0",
buzzinJohnnyBoi marked this conversation as resolved.
Show resolved Hide resolved
"dayjs": "~1.11.5",
"dev-null": "^0.1.1",
"dotenv": "~16.0.3",
Expand Down
27 changes: 20 additions & 7 deletions server/model/maintenance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,10 @@ class Maintenance extends BeanModel {
duration = this.duration * 1000;
}

if (duration === undefined && this.strategy === "recurring-interval") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need a bit of help understanding the if-clause you added 😅:
Is this not already covered by the if or else branch above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @CommanderStorm, so above is an if - else if - else, and the else if is when this.end_date is set, and it calculates if this.end_date will be before the end of the interval. If it is not, then this.duration is left undefined.
I guess my solution is not the best one, I was not sure if it was left undefined for some other purpose, but if it is not, then I guess the else if should look something like this:

else if (this.end_date) {
    let d = dayjs(this.end_date).diff(dayjs(), "second");
    if (d < this.duration) {
        duration = d * 1000;
    }
    else {
        duration = this.duration * 1000;
    }
}

Let me know if you want me to make this change.

Copy link
Collaborator

@CommanderStorm CommanderStorm Jul 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think that is better.
duration is never explicitly checked for undefined => I think it being undefined is a bug ^^

While you are at it, I think that while if/... clause would be way better if extracted into a function this.duration = this.inferDuration()
That way the controll flow is not as confusing in the future..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    inferDuration(customDuration) {
        // Check if duration is still in the window. If not, use the duration from the current time to the end of the window
        if (customDuration > 0) {
            return customDuration;
        } else if (this.end_date) {
            let d = dayjs(this.end_date).diff(dayjs(), "second");
            if (d < this.duration) {
                return d * 1000;
            }
        }
        return this.duration * 1000;
    }

Code here
Lmk if this is good.

duration = this.duration * 1000; // For recurring-interval, the duration needs to be defined
}

UptimeKumaServer.getInstance().sendMaintenanceListByUserID(this.user_id);

this.beanMeta.durationTimeout = setTimeout(() => {
Expand All @@ -263,9 +267,21 @@ class Maintenance extends BeanModel {
};

// Create Cron
this.beanMeta.job = new Cron(this.cron, {
timezone: await this.getTimezone(),
}, startEvent);
if (this.strategy === "recurring-interval") {
// For recurring-interval, Croner needs to have interval and startAt
const startDate = dayjs(this.startDate);
const [ hour, minute ] = this.startTime.split(":");
const startDateTime = startDate.hour(hour).minute(minute);
this.beanMeta.job = new Cron(this.cron, {
timezone: await this.getTimezone(),
interval: this.interval_day * 24 * 60 * 60,
startAt: startDateTime.toISOString(),
}, startEvent);
} else {
this.beanMeta.job = new Cron(this.cron, {
timezone: await this.getTimezone(),
}, startEvent);
}
buzzinJohnnyBoi marked this conversation as resolved.
Show resolved Hide resolved

// Continue if the maintenance is still in the window
let runningTimeslot = this.getRunningTimeslot();
Expand Down Expand Up @@ -395,10 +411,7 @@ class Maintenance extends BeanModel {
} else if (!this.strategy.startsWith("recurring-")) {
this.cron = "";
} else if (this.strategy === "recurring-interval") {
let array = this.start_time.split(":");
let hour = parseInt(array[0]);
let minute = parseInt(array[1]);
this.cron = minute + " " + hour + " */" + this.interval_day + " * *";
this.cron = "* * * * *"; // Because it is interval, it will be calculated in the run function
buzzinJohnnyBoi marked this conversation as resolved.
Show resolved Hide resolved
this.duration = this.calcDuration();
log.debug("maintenance", "Cron: " + this.cron);
log.debug("maintenance", "Duration: " + this.duration);
Expand Down
Loading