-
Notifications
You must be signed in to change notification settings - Fork 78
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
Support Tasks with custom parameters #413
Merged
+261
−11
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8cbe810
Add arguments column to maintenance_tasks_runs
adrianna-chang-shopify c15394e
Define API and add sample params task
adrianna-chang-shopify b4365a6
Tasks support parameters
adrianna-chang-shopify ff41ba6
System tests don't need to sleep in Tasks
adrianna-chang-shopify 0d548e3
Rescue and alert on ActiveRecord::ValueTooLong errors
adrianna-chang-shopify File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 changes: 6 additions & 0 deletions
6
db/migrate/20210517131953_add_arguments_to_maintenance_tasks_runs.rb
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# frozen_string_literal: true | ||
class AddArgumentsToMaintenanceTasksRuns < ActiveRecord::Migration[6.0] | ||
def change | ||
add_column(:maintenance_tasks_runs, :arguments, :text) | ||
adrianna-chang-shopify marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
# frozen_string_literal: true | ||
module Maintenance | ||
class ParamsTask < MaintenanceTasks::Task | ||
attribute :post_ids, :string | ||
adrianna-chang-shopify marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
validates :post_ids, | ||
presence: true, | ||
format: { with: /\A(\s?\d+(,\s?\d+\s?)*)\z/, allow_blank: true } | ||
|
||
class << self | ||
attr_accessor :fast_task | ||
end | ||
|
||
def collection | ||
Post.where(id: post_ids_array) | ||
end | ||
|
||
def count | ||
collection.count | ||
end | ||
|
||
def process(post) | ||
sleep(1) unless self.class.fast_task | ||
|
||
post.update!(content: "New content added on #{Time.now.utc}") | ||
end | ||
|
||
private | ||
|
||
def post_ids_array | ||
post_ids.split(",") | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Previously a wrong Task name would result in a
ActiveRecord::RecordInvalid
raised from Runner and error caught in the action, now it will raise here and will result in a 500. We don't have tests for this because we didn't want controller tests and there's no way to test this with the UI (with a browser test, you'd need to see the UI and remove the task before clicking on Run). We could ignore that for now because like I mentioned in a previous review, parts of this logic should move to the Runner, with the CLI job needing similar handling, and fix it once we support arguments from the CLI.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.
Hum, interesting. Let's leave it for now, I can remove it in the CLI PR if we choose to go that route and we feel it's redundant. Alternatively, we could keep the use of the Strong Parameters API here and just rescue
Task::NotFoundError
and return{}
. I'm suspecting that we may just want to check the keys invalidate_task_arguments
though, since we're already rescuing onTask::NotFoundError
there. But I'll defer to the CLI PR.