Problem/Motivation

Right now we have 1 setting automatic_updates.settings.cron which has 3 values disabled, patch and security
But since #3351895: Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth there are 2 ways to run cron

Proposed resolution

Settings

Remove automatic_updates.settings.cron in favor of 2 new settings

  1. automatic_updates.settings.unattended.method which will currently have 2 possible values: background, console

    I am not sure "background" is the best but I don't to be "cron" because in #3357969: For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits is going separate from cron.
    I would like "console" also not be "drush" so that if have both symfony console or drush options the admin doesn't have to switch the setting both should work if automatic_updates.settings.method === "console"

  2. automatic_updates.settings.unattended.level
    This would could be either: disabled, patch or security

\Drupal\automatic_updates\Validation\StatusChecker::run needs to be updated to run set the stage to the correct Drush stage or Cron stage or UpdateStage No it doesn't. See #8.

Hook requirements/status report

✅ Change how \Drupal\automatic_updates\Validation\StatusCheckRequirements::getRequirements to NOT run the status checks directly but used the cached version. This is because running the status checks as the web user would likely cause validation errors if the file system is not writable because the admin choose to use the console option. This will also help with #3359670: [PP-1] \Drupal\automatic_updates\Validation\StatusCheckRequirements makes admin/reports/status 10x slower!, since we would usually be using cached results.

✅ Only show the Rerun readiness checks now now link on the status report if the user has selected the "background"

AutomaticUpdatesCommands

✅ Update \Drupal\automatic_updates\Commands\AutomaticUpdatesCommands::autoUpdate to run the status checks which will cache them.
If there is an update the status checks should be run after the update. The same checks will be run in pre-create. If there is no update the status checks should always be run.

automatic_updates_cron

✅ In automatic_updates_cron we should only run the status if both these are true:

  1. automatic_updates.settings.method === "webserver"(or whatever name we use)
  2. The process is not CLI.

    The CLI run might have different file permissions

Remaining tasks

In #3284443: Enable unattended updates we would actually provide the form for these settings
and probably add some help text

User interface changes

API changes

Data model changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Issue summary: View changes
tedbow’s picture

Issue summary: View changes
tedbow’s picture

Priority: Normal » Major

phenaproxima made their first commit to this issue’s fork.

tedbow’s picture

Issue summary: View changes

Updated the summary to use "background" instead of "webrequest"

phenaproxima’s picture

Issue summary: View changes

\Drupal\automatic_updates\Validation\StatusChecker::run needs to be updated to run set the stage to the correct Drush stage or Cron stage or UpdateStage

This is not something we can do, because DrushUpdateStage only exists when running Drush (that's where the service is defined). I think it's okay if it stays the way it is -- the point is that it's checking for an unattended update, and CronUpdateStage fulfills that criterion, at least for now. (Remember, DrushUpdateStage is a very thin extension of CronUpdateStage.)

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Assigned: phenaproxima » tedbow
Status: Active » Needs review
tedbow’s picture

Status: Needs review » Needs work
Issue tags: +alpha-target

Looking good. left some comments

phenaproxima’s picture

Status: Needs work » Needs review
tedbow’s picture

Assigned: tedbow » phenaproxima
Status: Needs review » Needs work

@phenaproxima needs work. that change in CronHandler might affect the tests so not looking at them now

phenaproxima’s picture

Assigned: phenaproxima » tedbow
Status: Needs work » Needs review
wim leers’s picture

Issue tags: -alpha-target +alpha target
wim leers’s picture

Review posted.

wim leers’s picture

Turns out @phenaproxima already tried enums, didn't work out. Ah well.

wim leers’s picture

Assigned: tedbow » Unassigned
Status: Needs review » Needs work
phenaproxima’s picture

Assigned: Unassigned » phenaproxima
phenaproxima’s picture

Assigned: phenaproxima » tedbow
Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! 😊

It's really good to see this getting done, because as became very explicitly clear at #3362143-20: Use the rsync file syncer by default and subsequent comments, we really need these checks to work reliably in all possible setups! 👍

tedbow’s picture

Assigned: tedbow » phenaproxima
Status: Reviewed & tested by the community » Needs work

Looks good just 1 problem. See MR comment

tedbow’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Reviewed & tested by the community

Looks good, if tests pass!!!!

tedbow’s picture

Status: Reviewed & tested by the community » Needs work

Needs work.

The new function test points out a problem. Probably caused by the fact that we run the status checks when install modules.

tedbow’s picture

This is looks very good!

I think 1 thing that is missing is that `automatic_updates_cron()` we should return early if method === cli

this because this could be cron is being run by drush cron or another CLI method then running the status checks might get results that don't match when run by web and we don't want to cache those or email.

\Drupal\automatic_updates\Commands\AutomaticUpdatesCommands::autoUpdate already runs status checks so if they selected cli they cron should never need to be run by cron

This also will need a test

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

Looks great! Thanks @phenaproxima

  • phenaproxima committed 83415837 on 3.0.x
    Issue #3359727 by phenaproxima, tedbow: Add new setting for how...
phenaproxima’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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