Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add configuration changes section to 1.62 upgrade docs #13309

Closed
wants to merge 1 commit into from

Conversation

behrmann
Copy link
Contributor

Pull Request Checklist

As mentioned in #13189 it has become harder to follow config changes since the example config has been stripped down. @DMRobertson proposed adding these changes to the upgrade docs. So here a first attempt to do this for 1.62.0 just by look at the diff

git log -p v1.61.0..v1.62.0 -- docs/usage/configuration/config_documentation.md
  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@behrmann behrmann requested a review from a team as a code owner July 18, 2022 12:00
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

this should be in the changes file rather than the upgrade notes, unless we expect users to have to take any action due to this change (and if we do, we should tell them what that action is).

Signed-off-by: Jörg Behrmann <behrmann@physik.fu-berlin.de>
@behrmann
Copy link
Contributor Author

Changed as requested. I don't have too much of a preference (I follow both files), as long as it is documented in a not too-hard-to-follow way somewhere.

@behrmann
Copy link
Contributor Author

I assume putting this in CHANGES.md will also necessitate updating the contributing guide and the towncrier config in pyproject.toml?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

We've discussed this as a team, and I'm afraid we don't really see the value in it, so we are reluctant to commit to doing it for every release.

The downsides of having this section include:

  • it is extra work that the Synapse development team need to commit to doing and getting right every time (otherwise it is useless) - sometimes that is harder than it sounds with an open-source project.
  • It seems like extra clutter for the majority of readers who do not find this information useful.

I don't understand why you feel setting this information out in this way is helpful:

  • extra_well_known_client_content is a new feature which is already called out in the "Features" section. I don't see any value in repeating it with slightly different words.
  • For sync_response_cache_duration's new default: ideally, the change to sync behaviour would have been called out as a feature rather than being lost in the middle of "internal changes", but adding more changelog sections that we need to remember to fill out is only going to exacerbate the problem of entries ending up in the wrong changelog section. In any case, presumably one of the following is true:
    • you have customised the setting, in which case you presumably had a good reason for doing so, and the new default doesn't affect you.
    • you are happy to follow the Synapse maintainers' judgement as to the best setting, so have left it set at the default. Why would you not accept the new default?
    • You do not feel comfortable accepting changes from the Synapse maintainers without careful review. Some words in a changelog entry are unlikely to be much help in this case.

In short: what do you expect to do with this information if we were to make it available?


What I do think we should have is a note in the configuration manual about when each configuration option was added and/or how it has changed. (See account_threepid_delegates for an example of this). It is regrettable that that got forgotten for these changes, and I would welcome PRs to the documentation to correct that oversight.

@behrmann
Copy link
Contributor Author

In short: what do you expect to do with this information if we were to make it available?

My workflow until 1.61 was to upgrade synapse on a test machine and diff the included homeserver.yaml to see what had changed. This was a neat workflow because configuration options and documentation where in the same place.

I fully understand that this was not sustainable, since the config had grown a bit unwieldy. I'm fully onboard with not shipping the full config example anymore.

This does leave a bit of a vacuum though. The changelog is excellent, but very on point. It's not always clear from the entries, whether the config is affected and if so how without following the PRs themselves to review code changes or comments.

This leaves looking at the git log for changes in the relevent files (the docs, the config module itself), which is a bit cumbersome. What I would like is a single place where I can see whether I not only need to change something about the config (i.e. I need to take action), but also whether I can change something.

What I do think we should have is a note in the configuration manual about when each configuration option was added and/or how it has changed. (See account_threepid_delegates for an example of this). It is regrettable that that got forgotten for these changes, and I would welcome PRs to the documentation to correct that oversight.
In short: what do you expect to do with this information if we were to make it available?

I'd be completely fine with this solution. :) I actually proposed the very same thing in what prompted the comment I cited above.

@richvdh
Copy link
Member

richvdh commented Jul 18, 2022

diff the included homeserver.yaml to see what had changed.
...
It's not always clear from the entries, whether the config is affected

I'm afraid I still don't quite understand why you find it so important to understand if any configuration options have changed, as opposed to any other change in Synapse's behaviour?

I'd be completely fine with this solution

Great! Would you be happy to close this PR and open a new one against https://github.com/matrix-org/synapse/blob/develop/docs/usage/configuration/config_documentation.md for these changes?

@behrmann
Copy link
Contributor Author

I'm afraid I still don't quite understand why you find it so important to understand if any configuration options have changed, as opposed to any other change in Synapse's behaviour?

Because for me as an admin that's the "user"-visible change. :) I cannot follow the minutiae of every PR, so I'll step through the code only when my users complain to me, but I will follow the docs and the knobs that I'm given in the config (because they were deemed important enough to be changeable by me without having to touch the code). That's why I like it when it's easy to follow these changes.

Great! Would you be happy to close this PR and open a new one against https://github.com/matrix-org/synapse/blob/develop/docs/usage/configuration/config_documentation.md for these changes?

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants