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
automatic_updates.settings.unattended.methodwhich will currently have 2 possible values: background, consoleI 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 ifautomatic_updates.settings.method === "console"automatic_updates.settings.unattended.level
This would could be either: disabled, patch or security
No it doesn't. See #8.\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
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:
automatic_updates.settings.method === "webserver"(or whatever name we use)- 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
Issue fork automatic_updates-3359727
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
Comment #2
tedbowComment #3
tedbowComment #4
tedbowComment #7
tedbowUpdated the summary to use "background" instead of "webrequest"
Comment #8
phenaproximaThis 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.)
Comment #9
phenaproximaComment #10
phenaproximaComment #11
phenaproximaComment #12
phenaproximaComment #13
phenaproximaComment #14
tedbowLooking good. left some comments
Comment #15
phenaproximaComment #16
tedbow@phenaproxima needs work. that change in CronHandler might affect the tests so not looking at them now
Comment #17
phenaproximaComment #18
wim leersComment #19
wim leersReview posted.
Comment #20
wim leersTurns out @phenaproxima already tried enums, didn't work out. Ah well.
Comment #21
wim leersComment #22
phenaproximaComment #23
phenaproximaComment #24
wim leersLooks 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! 👍
Comment #25
tedbowLooks good just 1 problem. See MR comment
Comment #26
tedbowLooks good, if tests pass!!!!
Comment #27
tedbowNeeds work.
The new function test points out a problem. Probably caused by the fact that we run the status checks when install modules.
Comment #28
tedbowWe were going to do #3360656: For web cron updates run each stage life cycle phase in a different request first but now we have closed that issue favor trying #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 but that will not before 3.0.x Alpha
So this issue needs to updated for changes in #3363926: To prevent conflict and performance problems with other code that runs during cron, decorate the cron service instead of implementing hook_cron
The 2 ways to run unattended updates
Comment #29
tedbowThis is looks very good!
I think 1 thing that is missing is that `
automatic_updates_cron()` we should return early ifmethod === clithis because this could be cron is being run by
drush cronor another CLI method then running the status checks might get results that don't match when run byweband we don't want to cache those or email.\Drupal\automatic_updates\Commands\AutomaticUpdatesCommands::autoUpdatealready runs status checks so if they selectedclithey cron should never need to be run by cronThis also will need a test
Comment #30
tedbowLooks great! Thanks @phenaproxima
Comment #32
phenaproxima