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

Clarify the behavior of "internalDatabase" setting #369

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jggc
Copy link

@jggc jggc commented Apr 1, 2023

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

jggc added 3 commits April 1, 2023 11:19
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]>
@provokateurin
Copy link
Member

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.

@jessebot
Copy link
Collaborator

@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.

@jessebot
Copy link
Collaborator

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?

@provokateurin
Copy link
Member

@jessebot I don't have permissions to force merge. So the bump is required :/ Sorry for the noise

@@ -1,6 +1,6 @@
apiVersion: v2
name: nextcloud
version: 3.5.11
version: 3.5.12
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
version: 3.5.12
version: 3.5.13

Copy link
Collaborator

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.

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