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

Enable message retention configuration policy. #796

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brndnmtthws
Copy link

This allows people to enable message retention policies on their homeserver.

One note: there are a few extra features that are available, but I decided to err on the side of simplicity in the implementation. For example, purge_jobs takes an array with a bunch of possible parameters, but I just simplified it and stuck with the defaults. If preferred, I can change the settings so that purge_jobs uses an array for the config values instead of extracting the parameters.

@spantaleev
Copy link
Owner

Thank you! 👍

While this is very nice, I wonder if we should keep introducing more and more dedicated variables for controlling homeserver.yaml. There are hundreds of options there (some more flexible and harder to control than others).

Generally, it's nice if we can keep things simpler (less dedicated options in the playbook) and rely on the universal mechanism we have for extending the configuration - matrix_synapse_configuration_extension_yaml. Using that, anyone can override and inject any configuration key, and we won't have to keep growing the list of variables we define as part of the playbook.

On a related note, retention.enabled is a very unfortunate name for this configuration option (but that's the fault of the Synapse folks). It defaults to false, which seems to imply that nothing is retained at all. retention_policy.enabled may have been a better name.

@brndnmtthws
Copy link
Author

I'm a little unclear as to where your comment means you want me to update the PR or not. If there is something specific you want me to change, please let me know.

@brndnmtthws brndnmtthws force-pushed the enable-synapse-retention-policy branch 2 times, most recently from 0f56d50 to 0cad5d1 Compare January 19, 2021 15:37
@brndnmtthws brndnmtthws force-pushed the enable-synapse-retention-policy branch 2 times, most recently from 037c830 to a205a03 Compare February 1, 2021 14:46
Copy link

@robbyoconnor robbyoconnor left a comment

Choose a reason for hiding this comment

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

I love this!

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