- Issue created by @tedbow
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Clarifying that this is meant to address @pwolanin's request at #3284443-19: Enable unattended updates β and @xjm's request at π± Security review of secure signing components for package manager Active :
We should also finish the work to allow autoupdatesβ cron job to be triggered by a separate user for defense-in-depth
- πΊπΈUnited States dww
Iβm also very keen on this part of the problem. We spent a lot of effort in Update Manager to support both writable and not-writable web server configurations. Itβd be a real step backwards if all the new slickness only supported the writable case.
- @tedbow opened merge request.
- π¬π§United Kingdom catch
I can see advantages for having a separate command so it can be run more frequently than cron and is guaranteed to be run from the cli, but also doesn't drush cron also cover the basic use case?
- last update
almost 2 years ago 738 pass, 1 fail - last update
almost 2 years ago Build Successful - last update
almost 2 years ago Custom Commands Failed - last update
almost 2 years ago 9 pass, 2 fail - last update
almost 2 years ago 9 pass, 2 fail - last update
almost 2 years ago 9 pass, 2 fail - last update
almost 2 years ago 8 pass, 1 fail - last update
almost 2 years ago 745 pass, 1 fail - last update
almost 2 years ago 747 pass - last update
almost 2 years ago 749 pass, 2 fail - Status changed to Needs work
almost 2 years ago 3:40pm 18 April 2023 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
First round of review posted. I have some questions π
- last update
almost 2 years ago Custom Commands Failed - last update
almost 2 years ago CI aborted - last update
almost 2 years ago CI aborted - last update
almost 2 years ago 719 pass, 2 fail - last update
almost 2 years ago 719 pass, 2 fail - last update
almost 2 years ago 751 pass - πΊπΈUnited States tedbow Ithaca, NY, USA
This is failing for me with manual testing. The composer update appear to work but I get "Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup."
Also the UX is not great because I guessing there is message from Composer STager I am not seeing. I need to check if that is case from the UI too.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Also the UX is not great because I guessing there is message from Composer STager I am not seeing. I need to check if that is case from the UI too.
Given this, should we block this on π Ensure exceptions thrown by event subscribers are logged Fixed , which should be trivial to do and would make the testing here simpler?
- πΊπΈUnited States tedbow Ithaca, NY, USA
re #13
In this case the problem is not coming from any event subscribers. I created π Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
You linked to this issue π Which did you mean to link to?
- πΊπΈUnited States tedbow Ithaca, NY, USA
So I manually tested this with the changes in π Failure marker message does not contain exception message + backtrace if available Fixed and the message change to a much more meaningful
Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup. Caused by : Failed to copy "/tmp/.package_managerff2a5e14-d9a4-4e06-b0c4-59070929c370/8wDnJ-H-KtVd8_feJmTRN9Fzk7VI5PGep_xeF7qIGns/web/modules/contrib/automatic_updates/.git/objects/pack/pack-ee29fcbd4b76af52c22f4bfd60176f21b3f2391b.idx" to "/Users/ted.bowman/sites/au-drush-update3/web/modules/contrib/automatic_updates/.git/objects/pack/pack-ee29fcbd4b76af52c22f4bfd60176f21b3f2391b.idx"
The actual problem here of trying to copy the .git files I think is an existing issue but I will search the issue queue.
I will probably manually test by just deleting the .git directory first as I am pretty sure this error has nothing to with it being a drush command. Usually when I manually test I am not using a Composer
vcs
repo. So need to confirm but I think this would happen via the Update form also. - Status changed to Postponed
over 1 year ago 4:32pm 3 May 2023 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Clarifying that this is blocked on π Failure marker message does not contain exception message + backtrace if available Fixed .
- Status changed to Needs review
over 1 year ago 6:43pm 5 May 2023 - πΊπΈUnited States tedbow Ithaca, NY, USA
I don't think we need to block on π Failure marker message does not contain exception message + backtrace if available Fixed . It is an existing issue I just happen to find in this issue it would happen in any case where you had the marker file
- last update
over 1 year ago 803 pass - last update
over 1 year ago 803 pass - last update
over 1 year ago 803 pass - Assigned to phenaproxima
- πΊπΈUnited States tedbow Ithaca, NY, USA
@phenaproxima was going to change the drush command to have only 1 command and have a
--post-apply=[STAGE_KEY]
option. - πΊπΈUnited States tedbow Ithaca, NY, USA
I manually tested and it updated from 10.0.8 to 10.0.9. I actually had to manually change
StagedDatabaseUpdateValidator
because it flagged a change to system_requirments has a DB update when it was not. But that is known issue and comment on π Use static analysis to detect new update functions, to reduce false positives in StagedDBUpdateValidator Fixed with not about this caseI think we still don't want to run DB updates in this drush command as MVP because we intended it to be run in server crontab in an unattended way. But we may want a follow-up that would allowing running DB updates.
I will manually test again once @phenaproxima has made further changes
- First commit to issue fork.
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago CI aborted - last update
over 1 year ago CI aborted - last update
over 1 year ago 762 pass, 2 fail - last update
over 1 year ago 762 pass, 2 fail - last update
over 1 year ago 762 pass, 2 fail - last update
over 1 year ago 762 pass, 2 fail - last update
over 1 year ago 762 pass, 1 fail - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@tedbow in #21: sure β but your comment in #18 made it sound like it was a hard blocker to continue here. So I was just trying to be helpful π π
@tedbow in #24: EXCITING!!!!!!! π
Also, this MR is starting to look great!
AFAICT the post-apply gets called automatically thanks to
ConsoleCronUpdateStage::handleCron()
calling\Drupal\automatic_updates\CronUpdateStage::handleCron()
which calls\Drupal\automatic_updates\CronUpdateStage::performUpdate()
which calls$this->triggerPostApply()
all the way at the end? π€ What am I missing? - Status changed to Needs work
over 1 year ago 12:29pm 8 May 2023 - last update
over 1 year ago 768 pass - Assigned to tedbow
- Status changed to Needs review
over 1 year ago 2:36pm 8 May 2023 - last update
over 1 year ago 768 pass - Status changed to Needs work
over 1 year ago 3:46pm 8 May 2023 - πΊπΈUnited States tedbow Ithaca, NY, USA
Looking good! Just a few suggestions
- last update
over 1 year ago 768 pass - last update
over 1 year ago CI aborted - last update
over 1 year ago 768 pass - Status changed to Needs review
over 1 year ago 4:43pm 8 May 2023 - πΊπΈUnited States tedbow Ithaca, NY, USA
Needs follow-up
for timeout issue in the MR discussions - last update
over 1 year ago 768 pass - last update
over 1 year ago 768 pass - πΊπΈUnited States tedbow Ithaca, NY, USA
I think this issue is good. We already have π For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed which researching the different places the code can time out.
I am going to manually test this again
- Status changed to Needs work
over 1 year ago 7:08pm 8 May 2023 - last update
over 1 year ago 768 pass - last update
over 1 year ago 787 pass - πΊπΈUnited States tedbow Ithaca, NY, USA
Got a different error manually testing
[error] Update applied successfully but failed in post-apply: The command """ 'auto-update' '--post-apply' '--stage-id=gqDxp7iWi8LDO3vkqYDESoeNS53Aa1oFLLIfBH_LPpI' '--from-version=10.0.8' '--to-version=10.0.9'" failed. Exit Code: 127(Command not found) Working directory: /Users/ted.bowman/sites/au-drush/web Output: ================ Error Output: ================ sh: line 0: exec: : not found
guessing
$command[0] = $this->fileSystem->realpath($command[0]);
didn't work. Maybe `$command[0]` just is "drush"?Maybe we need the standard drush way for firing off another command. I think maybe
Drush::drush(Drush::aliasManager()->getSelf(), $command, $args);
- πΊπΈUnited States phenaproxima Massachusetts
Maybe we need the standard drush way for firing off another command
I agree, but that means we should probably pass the fully built-out Process object to
Stage::triggerPostApply()
, which I'm not entirely sure how to do... - last update
over 1 year ago 782 pass, 1 fail - last update
over 1 year ago 788 pass - Status changed to Needs review
over 1 year ago 4:54pm 10 May 2023 - last update
over 1 year ago 788 pass - πΊπΈUnited States tedbow Ithaca, NY, USA
I manually tested if from /web/sites/default and it worked update it but the only messages I saw was
[INFO] Updating Drupal core to 10.0.9. This may take a while.
So it seems like the
io
is still not coming back from post apply.I have an idea on this
- last update
over 1 year ago 788 pass - last update
over 1 year ago 788 pass - last update
over 1 year ago 788 pass - last update
over 1 year ago 788 pass - Status changed to RTBC
over 1 year ago 6:30pm 10 May 2023 - πΊπΈUnited States tedbow Ithaca, NY, USA
Looks good to me if tests pass!
- last update
over 1 year ago 788 pass -
phenaproxima β
committed 4b6cef8d on 3.0.x authored by
tedbow β
Issue #3351895 by tedbow, phenaproxima, pwolanin, xjm: Add command to...
-
phenaproxima β
committed 4b6cef8d on 3.0.x authored by
tedbow β
- Status changed to Fixed
over 1 year ago 7:18pm 10 May 2023 - Status changed to Needs review
over 1 year ago 11:58am 11 May 2023 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π₯³
I'm pleasantly surprised this landed yesterday! π€©
But β¦ it seems a bunch of threads were either left open or closed without explanation? π That makes it difficult for me (or anybody else) to understand the decision making processβ¦
I've noticed that at least one of the remarks in the threads which did not get resolved/explained did start being addressed in another issue: π For web server dependent unattended updates run the entire life cycle in a separate process that will not be affected by hosting time limits Fixed . Hence linking that.
- We'll need to update
\Drupal\automatic_updates\Development\Converter
to avoid addingdrush.services.yml
src/Commands/AutomaticUpdatesCommands.php
src/DrushUpdateStage.php
to the core MR. Feels like that's easiest to do here? Patch attached.
- This still needs a follow-up to make this work as a generic Symfony console command. This is a great first step, and we were able to leverage Drush to handle the bootstrapping for us. But Drupal core never includes any Drush commands, so we do need to do that second step too π
- There's information in the section that is missing from the follow-ups that are referenced there. We should move that into those existing issues to avoid it being forgotten.
- We'll need to update
-
phenaproxima β
committed e94377d7 on 3.0.x
Issue #3351895 follow-up by Wim Leers: Do not include Drush-related code...
-
phenaproxima β
committed e94377d7 on 3.0.x
- πΊπΈUnited States tedbow Ithaca, NY, USA
added π Add new setting for how unattended updates will be run Fixed as remaining task
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
I identified 3 follow-ups when I reopened this in #41:
- β update core MR converter β @phenaproxima did this in #42
- β follow-up to make this work as a generic Symfony console command
- β update issues listed in "remaining tasks"
Or am I just overlooking an already existing issue for making this work as a Symfony console command? π€
- Status changed to Fixed
over 1 year ago 4:29pm 15 May 2023 - πΊπΈUnited States tedbow Ithaca, NY, USA
- Issue was unassigned.
Automatically closed - issue fixed for 2 weeks with no activity.