Skip to content
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

ownCloud: Update to v10.15.0 #6225

Merged
merged 18 commits into from
Oct 11, 2024
Merged

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Sep 10, 2024

Description

This PR contains the following:

  1. Update ownCloud to version 10.15.0
  2. Use recommended MariaDB database (migration guide)
  3. Optimise initial ownCloud setup
  4. Refine scripts for backup restoration
  5. Fix scripts for uninstall validation

Fixes #

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Package update

@mreid-tt
Copy link
Contributor Author

hey @hgy59, recently you mentioned a validation check that was missing in #6224 (comment). With this new build I'm wondering what's the best way to solve this issue.

@hgy59
Copy link
Contributor

hgy59 commented Sep 10, 2024

hey @hgy59, recently you mentioned a validation check that was missing in #6224 (comment). With this new build I'm wondering what's the best way to solve this issue.

@mreid-tt you need a validate_preinst () function in service-setup.sh
Here you implement a part of the code in service_postinst titled with # Check backup file path.
If the user wants to restore from database, but the $filename does not match the expected prefix (i.e. the path is missing or the file does not exist or the file has not the expected name) then you echo an error message (to stdout) and stop the execution with "exit 1".
This will show the message to the user and prevent the installation (i.e. the wizard is still open and the user can navigate back to fix the path or select other options)
You find commented samples of validate_* in the service-setup.sh script in spk/demoservice/src

@hgy59
Copy link
Contributor

hgy59 commented Sep 10, 2024

BTW the demoservice package is a good playground for development.

@mreid-tt
Copy link
Contributor Author

@hgy59 thanks for the feedback. It's been a while since I worked on this so I forgot about these functions. Will make the suggested changes in a new commit and test.

@mreid-tt mreid-tt force-pushed the owncloud-update branch 2 times, most recently from 5807eda to bdc0d01 Compare September 10, 2024 21:59
@mreid-tt mreid-tt self-assigned this Sep 11, 2024
@mreid-tt
Copy link
Contributor Author

@hgy59 I believe the scripts are good now. I also took the time to optimise them to use #!/bin/sh commands. Let me know if you have any further suggestions before I merge it.

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Sep 19, 2024

@hgy59, I revisited the package and identified several long-standing warnings in the settings screen. After some investigation, I managed to resolve three of them: a cron warning, an HSTS warning, and a memory cache warning.

However, two issues remain: file locking and the use of SQLite for the database. With a clearer understanding of backing up and restoring MySQL/MariaDB databases, I'm considering switching back to MariaDB. I'd appreciate your thoughts on this.

For the file locking issue, we could potentially resolve it by using Redis. We already have a Redis package that users can configure manually, or I could integrate it into the setup wizard.

The current configuration aligns with the "Private Home Server with Low Access Rates" recommendation from the ownCloud configuration guide on caching. To support a "Small Server," we'd need to address these two issues. I'd value your input on whether this effort is worthwhile.

EDIT: Alternatively, we could update the Wiki with instructions like this one: ownCloud: Converting Database Type, so users can implement the changes themselves.

EDIT: I've tested a manual migration from SQLite to MariaDB, and it seems too complex to integrate into the upgrade scripts, particularly for DSM 6. After installing Redis, all warnings were resolved, and the interface performance improved significantly. I’m inclined to proceed with configuring this as the default setup. For the upgrade process, I suggest incorporating checks to block the upgrade until the user completes the manual database migration steps. I'd appreciate your thoughts on this.

@mreid-tt
Copy link
Contributor Author

@hgy59 I've made a major commit that transitions the backend database to MySQL. Unfortunately, automating the process through a script isn’t reliable at this time, so I’ve created a Wiki page with instructions for users to manually complete the steps before updating to this version. You can review it here: https://github.com/SynoCommunity/spksrc/wiki/ownCloud-Database-Migration. I'd appreciate your feedback on it.

@hgy59 hgy59 mentioned this pull request Oct 4, 2024
7 tasks
@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 8, 2024

@hgy59, @th0ma7, I'm ready to proceed with this but would appreciate your input on the database migration approach before moving forward. If there's no feedback, I'll plan to publish later this week and address any end-user issues as they arise.

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 9, 2024

This is really well documented ...

A few comments:

  • I would suggest offering the user to install mariadb from the repository first (otherwise probably done at package installation time) - explaining that would be helpful. Also a link to MariaDB wiki or something so people can have something that explain what this really is (for newbies)
  • I haven't checked your code but I would assume there is a major disclaimer at install time? ... I just checked and now unsure, at upgrade time you state it only support MySQL although this speak to migrating to MariaDB?
  • Set a random password for the database user. - most probably best practice (I haven't worked with DB in years), but I'd leave an option to set a user-defined passwd.
  • Note: If an error occurs, - unclear if this applies to DSM6 or DSM7 or both due to the vertical bar to the side?

yup, well document overall, nice job!

@mreid-tt
Copy link
Contributor Author

  • I would suggest offering the user to install mariadb from the repository first (otherwise probably done at package installation time) - explaining that would be helpful. Also a link to MariaDB wiki or something so people can have something that explain what this really is (for newbies)

Well it's listed as a pre-requisite in the Makefile so I'm not sure there is a way to show anything before, other than in the release notes or package description.

  • I haven't checked your code but I would assume there is a major disclaimer at install time? ... I just checked and now unsure, at upgrade time you state it only support MySQL although this speak to migrating to MariaDB?

If it's a fresh install it will default to MariaDB. The main challenge is someone moving from 10.14.0 to 10.15.0 who would be running on SQLite that would need to migrate their database to MySQL. Ah! I see your point about mentioning MySQL when I mean MariaDB, yes that language can be made more consistent.

  • Set a random password for the database user. - most probably best practice (I haven't worked with DB in years), but I'd leave an option to set a user-defined passwd.

This is the password for the application to connect to the database which is something the user will not be interacting with. The random password is a recommendation from the upstream repo's installation guide which I agree with. It cuts down on admin accounts to remember and manage for the user.

  • Note: If an error occurs, - unclear if this applies to DSM6 or DSM7 or both due to the vertical bar to the side?

The error could potentially occur with either DSM 6 or 7. So one thing which I saw consistently on both is if you don't turn maintenance mode off in the previous step the migration will fail and will leave a partially constructed database in MariaDB. This is why the easiest way to recover would be to just dump it and start the migration again.

yup, well document overall, nice job!

Yeah, I tried to make it as copy and paste as possible but my main concern was for users who may not be comfortable with command line.

@mreid-tt mreid-tt merged commit fb098da into SynoCommunity:master Oct 11, 2024
7 checks passed
@mreid-tt mreid-tt deleted the owncloud-update branch October 11, 2024 01:27
@mreid-tt mreid-tt added the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Oct 11, 2024
@mreid-tt mreid-tt mentioned this pull request Oct 13, 2024
6 tasks
@mreid-tt mreid-tt removed the status/published Published and activated (may take up to 48h until visible in DSM package manager) label Oct 13, 2024
@mreid-tt mreid-tt mentioned this pull request Oct 14, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants