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

Allow core self-updates in the UI #3271

Closed
quicksketch opened this Issue Sep 1, 2018 · 41 comments

Comments

Projects
None yet
8 participants
@quicksketch
Copy link
Member

quicksketch commented Sep 1, 2018

Describe your issue or idea

In #3105, we added the ability to update Backdrop core via the UI, but we disabled it in the UI by default. It can be re-enabled by manually modifying installer.settings.json in the active config directory. We should validate core update functionality once 1.11.0 is out and then consider enabling this option in the UI for a future release of Backdrop.

This is a sub-issue of the main automatic-updates issue at #2018.

screen shot 2018-12-06 at 9 10 13 pm


merged PR: backdrop/backdrop#2407
Follow up PR: backdrop/backdrop#2440

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Oct 4, 2018

@serundeputy has included this task as part of #414. We might separate that portion out and do it here separately.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 6, 2018

Since 414 is for making updates automatic (and I don't want to abandon that idea just yet) I think we should add the UI here in this issue.

Then we can close this issue for 1.12, and We can revisit automatic updates after the manual ones have been working (safely) for a year or so. And after we get package signing in.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 6, 2018

I'm having a hard time following the code in https://github.com/backdrop/backdrop/pull/2289/files. Where is the core "Update" button supposed to appear? And what is the meaning of the new core_update config value? Is that supposed to be for automatic, manual, or to enable the core "Update" button UI I haven't located yet? I would love some advice / diection @serundeputy if you get a chance :)

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Dec 6, 2018

@jenlampton Currently there is no UI for core update in 1.11.x; you have to manually (in code) set "core_update": 1 in update.settings.json.

Then you can update at: /admin/modules/update by checking the box for Backdrop core and proceeding like any other module update.

bcmsup

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 7, 2018

you have to manually (in code) set "core_update": 1 in update.settings.json.

I'd written an update hook for 1.12 that enabled that setting, but I'd had no idea what to do afer that.

Then you can update at: /admin/modules/update

Ah, it's on the modules page! :) Thank you.

@serundeputy

This comment has been minimized.

Copy link
Member

serundeputy commented Dec 7, 2018

I'd written an update hook for 1.12 that enabled that setting, but I'd had no idea what to do afer that.

fwiw; there is an update hook and a UI for it in the #414 PR.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 7, 2018

Yes, but automatic updates are a year or so away, and we want manual updates in 1.12, so I wanted to make a PR for (only) that here :)

/me goes and looks at the "UI" in the other issue...

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 7, 2018

Yes, but automatic updates are a year or so away, and we want manual updates in 1.12, so I wanted to make a PR for that here :)

I was under the impression that both would be made available in 1.12, but auto updates (#414) would have been set to "Manually only", and we'd then set the default to "Security releases" at a later version @jenlampton

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 7, 2018

I was under the impression that both would be made available in 1.12

I don't think it's a good idea to allow automatic updates if we don't even know that ANY updating will even work! I think we need lots of proof. At least a years worth, with a change this dangerous.

"Manually only"

This is a setting to CHECK for available updates, not to actually do them. Auto updates are a whole other thing (and are currently blocked by the package signing issue anyhow).

we'd then set the default to "Security releases" at a later version

This isn't even possible right now, nor have we concluded that we even want to make it posisble yet. :)

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 7, 2018

...yes but the sooner we expose the UI to people, the more testing and feedback we'll get sooner.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 7, 2018

...yes, that's why I'm working on this issue :) So we can get a manual update button, so that we can get some proof that updating works!

Once we know updating works, we can add the option to make the updates automatic.

We can't add an automatic option right away, or people will use it. And if updating doesn't work for some reason, they might automatically delete their entire site, (perhaps without even noticing for days or even weeks) and that would be bad.

We want people to delete their entire sites manually, because then at least they'll know what happened :)

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 7, 2018

I've created a PR if people want to take this for a spin. It includes two new checkboxes, one of which is disabled and currently does nothing (other than promote automatic updates), so it would be easy to replace with a select or whatever we need later.

screen shot 2018-12-06 at 9 10 13 pm

I've also done a massive cleanup on the rest of the update settings form, including:

  • Added fieldsets for "Update checking" "Update notifications" and "Self-Updates".
  • Added #descriptions for the fieldsets (pulled text from form element descriptions)
  • Used #states to properly hide and show fields that are not necessary or don't make sense
  • Made all language more human
  • Made all radio options into complete sentences
  • Replaced "your site" with this website in all instances

screen shot 2018-12-06 at 9 15 45 pm

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 7, 2018

Made all radio options into complete sentences

👍 for that, but I don't think that we are using periods at the end of checkbox/radio labels elsewhere.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 7, 2018

...wondering if it would be a safer option to be renaming the old core and then extracting the update in its place. We'd then let the end user manually delete the backup of the old core.

If they don't delete the old backups, worse that can happen is to run out of disk space (and given the size of core and the space offerings by hosting providers nowadays, that'd be rare). That's way safer than destructively deleting things.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 7, 2018

This looks great @jenlampton!!! 👍

@herbdool

This comment has been minimized.

Copy link

herbdool commented Dec 7, 2018

@klonos can you move your last comment to the related issue? Let's keep this focused on the UI.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 9, 2018

You are right @herbdool ...I have cross-posted my comment in #414 instead of moving it there, as I feel it still has value here, because the help text we are to introduce says "Allow this website to first delete its core, and then replace its core...". This help text will also need to be updated if we do change this to rename + manual delete.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 14, 2018

Just to follow up on the order of operations we covered in today's weekly meeting:

  1. merge this as-is
  2. fix the update process to include a backup - and update this text in that PR
  3. add the backup file to the files_managed table so they can be deleted via the UI
  4. maybe also add a setting so that backups are or are-not created for core and/or contrib
@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 14, 2018

This is mostly RTBC from me. I have left some comments in the PR for your consideration @jenlampton ...mainly user-facing string changes (labels and help text).

Perhaps also:

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 14, 2018

@jenlampton Great! As a detail, I noticed that the link in the Self Updates section which leads to https://backdropcms.org/file-permissions-and-ownership, results in "Access denied".

@quinnanya

This comment has been minimized.

Copy link

quinnanya commented Dec 30, 2018

I installed Backdrop 1.11.1 on MAMP, and manually replaced the two core mentioned in this PR (https://github.com/backdrop/backdrop/pull/2407/files) with the ones in that PR (I downloaded the files from Github and overwrote the versions in the Backdrop 1.11.1 filesystem.)

I saw there was a security update, and went to admin/reports/updates/settings. There was an option for turning on manual core updates. I checked that box and saved. Then when I went to the available updates page, I was able to download and install Backdrop 1.11.3 through the UI as if it were a regular module, no problems.

@herbdool

This comment has been minimized.

Copy link

herbdool commented Dec 30, 2018

@quinnanya did you run update.php right after applying the PR? I expect it would still be the same result, but difference would be that the checkbox would already be checked (or should be at least).

@quinnanya

This comment has been minimized.

Copy link

quinnanya commented Dec 30, 2018

@herbdool Nope, didn't run update.php until after the new version of Backdrop core was downloaded via the UI.

@herbdool

This comment has been minimized.

Copy link

herbdool commented Dec 30, 2018

Based on @quinnanya and @olafgrabienski feedback I think we could mark this as RTBC. I too had successfully trested the update process before. This PR only makes a minor change and it also seems to work

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Dec 31, 2018

I thought we needed to set the default value in installer.settings.json, but I forgot we already has a value set there! So with that in place I've merged backdrop/backdrop#2407 into 1.x for 1.12.0. Thanks @jenlampton, @serundeputy, @klonos, @herbdool, @olafgrabienski, @stpaultim, and @quinnanya!

However, after merging I realized that the default value in installer.settings.json is false but the the value we set in the update hook is true. We need to decide of core updates are enabled by default and then update both places to be the same.

I'm leaving this needs work for the minor follow-up. If this takes any significant work we need to make a new issue.

@stpaultim

This comment has been minimized.

Copy link
Member

stpaultim commented Dec 31, 2018

I'm assuming that you meant to reopen this issue, since it requires follow up work?

Also a reminder about: #3447

@stpaultim stpaultim reopened this Dec 31, 2018

@herbdool

This comment has been minimized.

Copy link

herbdool commented Dec 31, 2018

Going by the comments on the PR it seems that preference might be to leave it as FALSE for now.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 31, 2018

it seems that preference might be to leave it as FALSE for now.

If the preference is to leave it false, then we won't need the update hook at all. I've filed a new PR to remove it. backdrop/backdrop#2440

edit: I've also opened another follow-up issue to set the value to true, and added the milestone of 1.13. We can re-evaluate that milestone, and/or discuss if we want that in the other issue: #3453

Also repeating @stpaultim's reminder about #3447

@jenlampton jenlampton changed the title Enable core updates in the UI Allow core self-updates in the UI Dec 31, 2018

@herbdool

This comment has been minimized.

Copy link

herbdool commented Jan 1, 2019

Looks good

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jan 2, 2019

Great, merged the follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment