nextcloud
Clarify the behavior of "internalDatabase" setting
Clarify that internalDatabase must be enabled: false to activate other databases in nextcloud config. Simply enabling mariadb, external or postgres database will not actually configure nextcloud to use it unless internalDatabase.enabled is false.
Pull Request
Description of the change
Only some clarifying comments
Benefits
Saves some confusion that requires newcomers to go and read the templates to understand what the internalDatabase setting actually does
Possible drawbacks
Adds 4 lines of comments in values.yaml, makes for a longer file, uses more space in the git repo and every clone. Should not cause too many issues in this world where terabytes of data is cheap.
Applicable issues
- fixes my nextcloud deployment that had a mariadb database running but was still using the sqlite database since I thought that "internalDatabase" could mean a database controlled by the helm chart, since "externalDatabase" means an external mysql/postgres database running outstide of the chart.
Additional information
I'm kind of having fun writing way too much information about this 4 line comment only change. But if you want some additional information I think I may take the time to rename the "internalDatabase" setting to something that depicts more obviously that this setting is the master setting that controls the sqlite database. Or better yet, have a setting that makes more explicit that it is controlling nextcloud database connection configuration.
Checklist
- [x] DCO has been signed off on the commit.
- [x] Chart version bumped in
Chart.yamlaccording to semver. - [x] (optional) Variables are documented in the README.md
Looks good, but I don't think you need to bump the chart version, so it's better to leave it as it only adds some comments.
@jggc you'll need to rebase to resolve the conflicts on the readme (the readme is now broken into different sections such as this database configuration section).
Then as per @provokateurin, you can also remove the chart version bump.
Looks good, but I don't think you need to bump the chart version, so it's better to leave it as it only adds some comments.
we can't do that yet, as it still violates the checks we have in place. can you squash and merge anyway, @provokateurin?
@jessebot I don't have permissions to force merge. So the bump is required :/ Sorry for the noise