danb35 / freenas-iocage-nextcloud

Script to create an iocage jail on FreeNAS for the latest Nextcloud 28 release, including Caddy, MariaDB or PostgreSQL, and Let's Encrypt
GNU General Public License v3.0
258 stars 71 forks source link

Config default bugfix, Tweaks and Reinstall option #96

Closed PrivatePuffin closed 4 years ago

PrivatePuffin commented 4 years ago

This PR covers a number of changes:

Notes: Added reinstall option (if config and DB are already present) The reinstall automatically triggers if an existing config is detected, no users intervention or variable is required. It also checks of a compatible database as the one set in the variable is detected and only continues if BOTH are true.

Renamed mariadb mountpoint folder Uniformity is important, the only place where mysql should be refered is the (odd, legacy) /var/db/mysql reference. renaming it to mariadb makes it play well with existing variables and the reinstall checks.

Fixed bug in config folder default I fucked up and looked over setting the default for the config folder. this should NOT set the files folder, which could corrupt the whole install. Fixed here

Added "themes folder" mountpoint To do a reinstall (or any form of saving config/data on external datasets), we should follow the Nextcloud backup guidelines: https://docs.nextcloud.com/server/stable/admin_manual/maintenance/backup.html?highlight=backup

Those dictate that we should also make a mountpoint for the themes folder, which I did.

Splitting nextcloud download + webserver config from from nextcloud install chapter It seems this was already pretty well seperated in code. The reason to add a chapter marker is the following: The only difference between an install and a reinstall of Nextcloud, is the fact that database and nextcloud config scripts/commands aren't called. Adding a chapter makers to seperate config of databases and nextcloud, creates clearity when developers read the reinstall if statement located there.

Explaining the new folder structure in the Readme It seems I forgot to explain the new folder structure (including the new default nextcloud parent dataset) in the readme. This fixes that and also adds instructions for the new themes sub-dataset

Create in-code chapter for results output The added reinstall option adds some logic to the console output, which makes it worthwhile giving it a clear chapter tag.

PrivatePuffin commented 4 years ago

This work was heavily inspired by my work on my automated jail manager (jailman), which includes a modified version of this script and uses these changes to support reinstall: https://github.com/Ornias1993/jailman/tree/jails/nextcloud

Besides some basic testing here, above version (using the generally same way of defining the reinstall) was tested about 20 times, installing and reinstalling without issues.

PrivatePuffin commented 4 years ago

@danb35 If you are not interested in merging, please just say so and close the PR so I can delete my branch.

PrivatePuffin commented 4 years ago

I'll make sure it's rebased, just in case!

danb35 commented 4 years ago

I was just about to respond and ask that you do that--I know I'd already fixed the FILES/CONFIG path bug, but I'm not sure where else it's conflicting.

PrivatePuffin commented 4 years ago

@danb35 Github is confused it seems... thinks you removed what I just added... rebase should fix it... incomming!

PrivatePuffin commented 4 years ago

Done, rebase is in.

danb35 commented 4 years ago

It seems to be working for me. Could you maybe add a paragraph or two to the README explaining how the reinstall works? It looks like the reinstallation happens if $CONFIG_PATH and $DB_PATH/$DATABASE exist and have contents, and assumes (reasonably, I think) that those contents are valid Nextcloud config and database files, respectively. So I guess if $FILES_PATH exists and has contents, users in the reinstallation would see them and have their files available; if not, they start over? And the same thing for themes?

I do notice one issue: $DB_PATH/$DATABASE and $CONFIG_PATH don't get created--could you add that, around line 210?

PrivatePuffin commented 4 years ago

Will do those last two things. Indeed it should.

That being said: It follows the guidelines from nextcloud: If you keep those folders seperate, and copy/move them to a new install, all users should keep their files and settings :)

Because you don't want to create a new database and config file, it needs to skip that step if those files already exist... But yes, it does assume those are valid files, they might not... but I think if users manage to corrupt files and/or databases, that we can shout "user error" as there is no way to validate those... (even nextcloud doesn't btw)

danb35 commented 4 years ago

$THEMES_PATH should also be created.

PrivatePuffin commented 4 years ago

Ohh, Now I get this. No It doesn't create it. Because people should make datasets or folders for those (as instructed in the readme). Actually they shouldn't use folders for it in the first place.