The original architecture of the site included an option for grouping system monitors and then allowing those monitors to be run via drush by those groups.

However, the concept of a "group" seems exclusive and it would make more sense to allow the grouping of monitors and tasks based on a tagging system. This would allow users to include a single monitor or task within multiple tags, providing more flexibility when runnings and displaying results.

Status pages will still display System Monitors based on their grouping and active tasks therein, regardless of tagging. The tagging system will purely be for organizing tasks and monitors into groups that can be triggered via drush commands.

If a task is not active against a system monitor, then its tags will be ignored since that task would not be considered active in the system. Setting a tag at the Monitor level is all that is required to set the tag for all of that monitor's tasks. Setting a tag against a task is only necessary when that task needs to be able to be triggered outside of the schedule of its parent System Monitor.

Requirements for this ticket

  • Replace "group" with "tags" on schema of system monitor entity.
  • Update System Monitor entity form to use new comma-separated, tagging field. (no autocomplete is necessary)
  • Update System Monitor Task annotations to allow for declaring tags at the task level
CommentFileSizeAuthor
#5 system_monitor--convert-group-to-tags--3098561--5.patch7.53 KBmerauluka
#2 system_monitor--convert-group-to-tags--3098561--2.patch6.44 KBmerauluka
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merauluka created an issue. See original summary.

merauluka’s picture

merauluka CreditAttribution: merauluka at Mediacurrent commented
Status: Active » Needs review
FileSize
system_monitor--convert-group-to-tags--3098561--2.patch6.44 KB

Attached is a patch to convert from a simple, single string group to multi-value tags on both system monitor configs and in the Annotations on system monitor tasks.

Comparison in Gitlab:
https://git.drupalcode.org/project/system_monitor/compare/8.x-1.x...task...

robpowell’s picture

robpowell
he/him
Location Boston
CreditAttribution: robpowell at Mediacurrent commented
Assigned: Unassigned » merauluka
Status: Needs review » Needs work
robpowell’s picture

robpowell
he/him
Location Boston
CreditAttribution: robpowell at Mediacurrent commented

+ * A list of comma-separated values used to group tasks during cron jobs.
It says list but is actually based off of \n.

/**
-   * Get the group to use when displaying the system monitor on the status page.
+   * Get the tags to use when running the system monitor via drush commands.
    *
-   * @return string
-   *   The group of the System Monitor.
+   * @param bool $return_string
+   *   Whether or not to return an array or a string. Default to an array.
+   *
+   * @return array|string
+   *   The tags for the System Monitor.
    */

you can use {@inheritdoc} since it is getting this method from the interface.

I also believe SystemMonitorTaskManager::runSystemMonitorTasks() needs to be updated to use tags and not groups.

merauluka’s picture

merauluka CreditAttribution: merauluka at Mediacurrent commented
Status: Needs work » Needs review
FileSize
system_monitor--convert-group-to-tags--3098561--5.patch7.53 KB

Nice catch @robpowell. I've updated the patch based on your feedback.

Comparison in Gitlab:
https://git.drupalcode.org/project/system_monitor/compare/8.x-1.x...task...

robpowell’s picture

robpowell
he/him
Location Boston
CreditAttribution: robpowell at Mediacurrent commented

Looks like you address #3 but not #2 and #1 in comment #4. Let me know if you think those should be skipped or if you'd like me to run with it.

merauluka’s picture

merauluka CreditAttribution: merauluka at Mediacurrent commented
Assigned: merauluka » robpowell

@robpowell please go ahead and update that if you have the bandwidth.

robpowell’s picture

robpowell
he/him
Location Boston
CreditAttribution: robpowell at Mediacurrent commented
Status: Needs review » Needs work

  • merauluka committed 3a064eb on 8.x-1.x 
    Issue #3098561 by merauluka, robpowell: Add support for "tags" instead...
merauluka’s picture

merauluka CreditAttribution: merauluka at Mediacurrent commented
Assigned: robpowell » Unassigned
Status: Needs work » Fixed

Discussed with @robpowell today over a slack call and we determined that the changes requested had been made.

Merging into the codebase and marking as Fixed.

  • merauluka committed 3a064eb on 9.x-1.x 
    Issue #3098561 by merauluka, robpowell: Add support for "tags" instead...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.