Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

[dendrite] Per-component database settings #1581

Merged
merged 2 commits into from
Jun 2, 2022
Merged

[dendrite] Per-component database settings #1581

merged 2 commits into from
Jun 2, 2022

Conversation

Omar007
Copy link
Contributor

@Omar007 Omar007 commented May 22, 2022

Description of the change

This change allows database configuration to be set for each of the sub-components of Dendrite.

Benefits

This enabled use-cases such as:

  • Use of a database other than SQLite or the included postgresql deployment
  • Configure a completely separate database target for each component
  • Configure database connection parameters per component

Possible drawbacks

None that I'm aware of. Backward compatibility is kept with the current logic.

Applicable issues

N/A

Additional information

N/A

Checklist

  • Title of the PR starts with chart name (e.g. [home-assistant])
  • Chart version bumped in Chart.yaml according to semver.
  • Chart artifacthub.io/changes changelog annotation has been updated in Chart.yaml. See Artifact Hub documentation for more info.
  • Variables have been documented in the values.yaml file.

@ghost ghost added size/L Categorises a PR that changes 100-499 lines, ignoring generated files. precommit:ok CI status: pre-commit validation successful changelog:ok CI status: changelog validation successful lint:ok CI status: linting successful install:failed CI status: install failed labels May 22, 2022
@Omar007
Copy link
Contributor Author

Omar007 commented May 23, 2022

Looks like the CI is failing on something unrelated to this change? 🤔

time="2022-05-23T06:09:42Z" level=info msg="Starting \"syncapi\" component"
time="2022-05-23T06:09:42Z" level=error msg="Configuration error: You have tried to enable open registration without any secondary verification methods (such as reCAPTCHA). By enabling open registration, you are SIGNIFICANTLY increasing the risk that your server will be used to send spam or abuse, and may result in your server being banned from some rooms. If you are ABSOLUTELY CERTAIN you want to do this, start Dendrite with the -really-enable-open-registration command line flag. Otherwise, you should set the registration_disabled option in your Dendrite config."
time="2022-05-23T06:09:42Z" level=fatal msg="Failed to start due to configuration errors"

@Jonnobrow
Copy link
Contributor

Jonnobrow commented May 29, 2022 •
edited
Loading

For some reason the syncapi is pulling the latest version rather than v0.8.1. In v0.8.3 there was a modification to make open registration harder.

So to fix:

  • Pin syncapi version
  • and Fix the config to add in the missing fields and bump AppVersion to v0.8.6

EDIT: Seem like an easy fix, I must have added it by mistake here: https://github.com/k8s-at-home/charts/pull/1451/files

@Jonnobrow
Copy link
Contributor

I have opened a PR with a fix: https://github.com/Omar007/charts/pull/1

@Omar007
Copy link
Contributor Author

Omar007 commented May 30, 2022

I have cherry-picked the one commit with the changes for dendrite out of that PR and added it here.

@ghost ghost added lint:failed CI status: linting failed and removed lint:ok CI status: linting successful labels May 30, 2022
Jonnobrow
Jonnobrow reviewed May 30, 2022
View reviewed changes
Copy link
Contributor

@Jonnobrow Jonnobrow left a comment

Choose a reason for hiding this comment

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

Couple of things I missed by the looks of it

charts/incubator/dendrite/values.yaml Show resolved Hide resolved
charts/incubator/dendrite/values.yaml Show resolved Hide resolved
@ghost ghost added precommit:failed CI status: pre-commit validation failed precommit:ok CI status: pre-commit validation successful changelog:ok CI status: changelog validation successful lint:ok CI status: linting successful install:ok CI status: install successful and removed precommit:ok CI status: pre-commit validation successful changelog:ok CI status: changelog validation successful lint:failed CI status: linting failed install:failed CI status: install failed precommit:failed CI status: pre-commit validation failed labels May 30, 2022
@truxnell
Copy link
Member

truxnell commented Jun 1, 2022

LGTM. However, are we confident it wont be breaking to users - i.e. is a minor bump appropriate?
It does look to me like it wont be breaking however i do note this PR would change 'registration_disabled' from false to true for users?

@Omar007
Copy link
Contributor Author

Omar007 commented Jun 1, 2022

Yea I suppose with those fixes maybe the version should be just bumped to 5.0.0 🤔

@Omar007
[dendrite] Allow per-component database configuration
cd6d89a 
Enable database configuration to be provided on a per-api level.
If not manually configured/specified, default to the global settings.
Backward compatibility with current configuration logic kept.

Signed-off-by: Omar Pakker <[email protected]>
truxnell
truxnell suggested changes Jun 2, 2022
View reviewed changes
charts/incubator/dendrite/Chart.yaml Outdated Show resolved Hide resolved
charts/incubator/dendrite/Chart.yaml Show resolved Hide resolved
@truxnell
Copy link
Member

truxnell commented Jun 2, 2022 •
edited
Loading

I may be taking a over-abundance of caution, but do we do similar to the review, a major semver + callout the registration change?
Happy to go either way, I'm not familiar with the chart, but we do like to follow SemVer closely.

@Jonnobrow @truxnell @Omar007
[dendrite] CI fix & dendrite update (v0.8.7)
d5c5274 
- Fix syncapi tag pinning (actually default to chart.appVersion as documented)
- Change registration_disabled default value from false to true (see config failure CI runs)
- Add report_stats properties
- Add bcrypt_cost config property for userapi
- Major bump due to default change

Co-authored-by: Jonathan Bartlett <[email protected]>
Co-authored-by: Truxnell <[email protected]>
Signed-off-by: Omar Pakker <[email protected]>
@Omar007
Copy link
Contributor Author

Omar007 commented Jun 2, 2022

I have bumped it up to 0.8.7 due do some important fixes in Dendrite itself and included both chart suggestions bumping it to 5.0.0 and explicitly mentioning the defaults change in the changelog 👍

@truxnell truxnell merged commit 22ec029 into k8s-at-home:master Jun 2, 2022
@truxnell
Copy link
Member

truxnell commented Jun 2, 2022 •
edited
Loading

Thanks for the PR @Omar007 & assistance @Jonnobrow!

@truxnell
Copy link
Member

truxnell commented Jun 2, 2022

@all-contributors, can you add @Omar007 for code.

@truxnell
Copy link
Member

truxnell commented Jun 3, 2022 •
edited
Loading

I summon thee from the firey depth of hell @all-contributors!
By thine divine glory add @Omar007 for code contributions to this, our most holy of repositories.

@allcontributors allcontributors bot mentioned this pull request Jun 3, 2022
Merged
@allcontributors
Copy link
Contributor

@truxnell

I've put up a pull request to add @Omar007! 🎉

@truxnell
Copy link
Member

truxnell commented Jun 3, 2022

And now begone, to thence which you have came @all-contributors!
I banish thee, once, twice, thrice! Abandon this plane of existence and leave us mortals in peace.
I bind yee to the nine trees of eternity once more until my wishes desire it.

@Omar007 Omar007 deleted the feature/per-api-db-settings branch June 3, 2022 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Jonnobrow Jonnobrow Jonnobrow left review comments

@truxnell truxnell truxnell approved these changes

Assignees
No one assigned
Labels
changelog:ok CI status: changelog validation successful install:ok CI status: install successful lint:ok CI status: linting successful precommit:ok CI status: pre-commit validation successful size/L Categorises a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Omar007 @Jonnobrow @truxnell