-
Notifications
You must be signed in to change notification settings - Fork 108
Introduce support for task staleness threshold #1406
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,6 +471,15 @@ def configure_cursor_encoding! | |
| self.cursor_is_json = true | ||
| end | ||
|
|
||
| # Returns whether the run is beyond the outdated task threshold. | ||
| # | ||
| # @return [Boolean] | ||
| def outdated? | ||
| return false unless ended_at.present? | ||
|
|
||
| ended_at < MaintenanceTasks.outdated_task_threshold.ago | ||
|
||
| end | ||
|
|
||
| private | ||
|
|
||
| def instrument_status_change | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -64,6 +64,8 @@ def initialize(name, related_run = nil) | |||||
|
|
||||||
| alias_method :to_s, :name | ||||||
|
|
||||||
| delegate :outdated?, to: :related_run | ||||||
|
||||||
| delegate :outdated?, to: :related_run | |
| delegate :outdated?, to: :related_run, allow_nil: true |
This would raise if related_run is nil. Can we also add a test for if there is no run?
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.
Great catch! I didn't catch the default arg of nil above. I wasn't able to get allow_nil: true to work how I wanted since it would return nil for the TaskDataIndex#stale? delegation where I want it to be a Boolean. That said maybe there is a default value that could be set using this (couldn't find anything in the docs).
I'm making an update to remove the delegation in favor of a full method definition which handles the above nicely.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,6 +137,14 @@ module MaintenanceTasks | |
| # @return [Boolean] whether or not to store cursor values as JSON. | ||
| mattr_accessor :serialize_cursors_as_json, default: false | ||
|
|
||
| # @!attribute outdated_task_threshold | ||
| # @scope class | ||
| # The threshold after which a task is considered outdated. | ||
| # Defaults to 30 days. | ||
| # | ||
| # @return [ActiveSupport::Duration] the threshold after which a task is considered outdated. | ||
| mattr_accessor :outdated_task_threshold, default: 30.days | ||
|
||
|
|
||
| class << self | ||
| DEPRECATION_MESSAGE = "MaintenanceTasks.error_handler is deprecated and will be removed in the 3.0 release. " \ | ||
| "Instead, reports will be sent to the Rails error reporter. Do not set a handler and subscribe " \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Maintenance | ||
| class OutdatedTask < MaintenanceTasks::Task | ||
| def collection | ||
| [1, 2] | ||
| end | ||
|
|
||
| def process(number) | ||
| Rails.logger.debug("This task is outdated") | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,18 @@ class TasksTest < ApplicationSystemTestCase | |
| assert_text(/January 01, 2020 01:00 Succeeded #\d/) | ||
| end | ||
|
|
||
| test "show a Task with outdated run" do | ||
| travel_to(maintenance_tasks_runs(:outdated_task).ended_at + 2.days) do | ||
| MaintenanceTasks.with(outdated_task_threshold: 1.day) do | ||
| visit maintenance_tasks_path | ||
|
|
||
| within page.find("a", text: "Maintenance::OutdatedTask").find(:xpath, "..").sibling(".has-text-warning") do |element| | ||
| assert_text "This task last ran 1 day ago. Consider removing it as it may be outdated." | ||
| end | ||
|
||
| end | ||
| end | ||
| end | ||
|
|
||
| test "task with attributes renders default values on the form" do | ||
| visit maintenance_tasks_path | ||
|
|
||
|
|
||
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.
outdateddoesn't seem like the best fit for this feature as it implies that the task should be updated while I believe we want to convey that the task was run and may not be used anymore.How about
stale?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 love the clarity here. I'll make the update to
staleterminology 🎉