-
Notifications
You must be signed in to change notification settings - Fork 269
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
Clarify the behavior of "internalDatabase" setting #369
base: main
Are you sure you want to change the base?
Conversation
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. Signed-off-by: Jean-Gab <[email protected]>
Signed-off-by: Jean-Gab <[email protected]>
Signed-off-by: Jean-Gab <[email protected]>
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. |
Signed-off-by: JesseBot <[email protected]>
Signed-off-by: JesseBot <[email protected]>
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 |
charts/nextcloud/Chart.yaml
Outdated
@@ -1,6 +1,6 @@ | |||
apiVersion: v2 | |||
name: nextcloud | |||
version: 3.5.11 | |||
version: 3.5.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version: 3.5.12 | |
version: 3.5.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't do a sign off and commit on an old commit.
@jggc if you can bump the chart version , we can get this merged. Sorry for the confusion.
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
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
Chart.yaml
according to semver.