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

Revert #2698 forget_rooms_on_leave to upstream value #2700

Closed
wants to merge 1 commit into from

Conversation

FSG-Cat
Copy link
Contributor

@FSG-Cat FSG-Cat commented May 24, 2023

Reverts forget_rooms_on_leave to upstream as this is too dangerous to change in a breaking way like this is doing. Especially doing it together with a Call to Action to update your Synapses causing this change to enter all playbook users playbooks who update.

Its safe to keep the version bump part of #2698 but not safe to keep this change that this commit is undoing by setting the value to false. This means our config now includes the value like a upstream config would but we keep it at false just like upstream does.

This value is recomended to be made easily accessible and a Changelog note about this should be noted so that users know this setting exists if they want to use it as we should have a note explaining this event happened either way.

Reverts forget_rooms_on_leave to upstream as this is too dangerous to change in a breaking way like this is doing. Especially doing it together with a Call to Action to update your Synapses causing this change to enter all playbook users playbooks who update.

Its safe to keep the version bump part of spantaleev#2698 but not safe to keep this change that this commit is undoing by setting the value to false. This means our config now includes the value like a upstream config would but we keep it at false just like upstream does.
@spantaleev
Copy link
Owner

I added that it is s breaking change and that we generally stick to upstream defaults for the most part. Thus, a changelog entry should probably be added.

However:

  • the upstream defaults for this are probably unreasonable. Synapse is wasteful and us making it a tiny bit better is probably good

  • few people likely care about this. Most people will probably even want for the room to be forgotten

  • a changelog entry is probably better than reverting it

@FSG-Cat
Copy link
Contributor Author

FSG-Cat commented May 24, 2023

I am personally fine with the change existing if clearly documented as it means you can just change it if you disagree with the setting. And since its clear this change is going to say we can close this PR.

@FSG-Cat FSG-Cat closed this May 24, 2023
spantaleev added a commit that referenced this pull request May 25, 2023
@spantaleev
Copy link
Owner

This is now documented in c55371e.

I should have done it yesterday, but I was away from my computer when I merged that PR and decided to be lazy about it and just merge the change without a changelog entry, even though I believe it's necessary to add one. I appologize for this omission.

KarolosLykos pushed a commit to KarolosLykos/matrix-docker-ansible-deploy that referenced this pull request Mar 5, 2024
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.

2 participants