-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Check version number asynchronously #3185
Conversation
I had the bright idea of creating this update script but it doesn't work and hasn't been maintained, so it's just sitting there causing confusing and requiring weird things to exist in other places. Now, the unused `version` field can be removed and I can scrap the management command for getting versions.
@bookwyrm-social/code-review |
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.
This looks really nice and works well (I tested by adjusting the frequency to 1 minute, and got the alert box in the dashboard). I think it's a real improvement to show all the scheduled tasks on a dashboard.
A couple of comments, but they're not show-stoppers, just suggestions really.
{{ task.interval.id }} | ||
</td> | ||
<td> | ||
{{ task.enabled|yesno }} |
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.
Currently there does not seem to be a way to disable celery.backend_cleanup
or check-for-updates
.
Now, one might argue that they shouldn't be disabled, but it seems a bit odd that you have to manually turn on check-for-updates
but then you can't turn it off.
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.
yeah, I agree, I think it should be a subsequent PR because celery.backend_cleanup might be the kind of the thing that it's Not Good to turn off
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.
actually but now that I saw that, if someone has it running every minute, they're probably going to be displeased not to be able to stop 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.
I guess we could trigger a big scary banner if it's ever turned off. I don't think this needs to be a blocker for this PR, just wanted to note it for future improvement.
<table class="table is-striped is-fullwidth"> | ||
<tr> | ||
<th> | ||
{% trans "Name" %} |
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.
A nice-to-have (possibly in a subsequent PR) would be to link the task name to some sort of explanation of the purpose of the task and/or what it does.
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.
Periodic tasks have a description
field which we're currently not using, but presumably would be perfect for this
Admins will now need to schedule daily checks for updated releases. The UI for that looks like:
This does a few other cleanup tasks to remove unused version code and create a view for seeing a table of scheduled tasks:
Works on #2717