Allow non-interactive config generation from dpkg config snippets

[behrmann]

Pull Request Checklist

Somehow I couldn't get #13615 to work for me without specifying the server name or whether to report stats, but at least in the Debian installation they are already there, so this does a little trick to look into those special files (and to put them to some use, because on my machines they were mostly superfluous).

This isn't done yet, probably, because this should allow to drop shipping the example config in the Debian installation, but I haven't tried this yet. I did want to gather feedback on this, though.

• 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)

[richvdh]

I think I'm failing to understand your ultimate goal here. It seems you want to get away from including a homeserver.yaml in the debian package (and, presumably, you want to stop the package generating /etc/matrix-synapse/conf.d/server_name.yaml etc), but why is this an improvement?

[richvdh]

Setting aside for now the fact that we're hardcoding some very debian-package-specific magic into the main code here:

It's worth noting that, at present, you can dpkg-reconfigure matrix-synapse-py3 and change the report_stats setting. That will be written back to /etc/matrix-synapse/conf.d/report_stats.yaml so that it is picked up by synapse on the next restart.

It looks to me as if this change will put the report_stats setting into /etc/matrix-synapse/homeserver.yaml on first run, where it will never be changed thereafter - so users can no longer dpkg-reconfigure this setting.

[behrmann]

That's a good point, I'll think about how to address this.

[richvdh]

Somehow I couldn't get #13615 to work for me without specifying the server name or whether to report stats

I think you might be misunderstanding what #13615 does (and this might explain my confusion about your comments on that PR).

Even with #13615 in place, Synapse will not create a top-level homeserver.yaml configuration file (unless you tell it to do so explicitly with --generate-config). Rather, it will generate any secondary configuration files referenced from that primary config file (via registration_shared_secret_path, signing_key_path or log_config).

[behrmann]

I think I'm failing to understand your ultimate goal here. It seems you want to get away from including a homeserver.yaml in the debian package [..], but why is this an improvement?

Agreed. Looking at #13615, I see that I misunderstood it. I just tried a slightly changed service file with

ExecStartPre=!/opt/venvs/matrix-synapse/bin/python -m synapse.app.homeserver --config-path=/etc/matrix-synapse/conf.d --generate-missing-configs
ExecStart=/opt/venvs/matrix-synapse/bin/python -m synapse.app.homeserver --config-path=/etc/matrix-synapse/conf.d 

leaving the homeserver.yaml out and using only the server name and report stats snippets. This generates the signing key but then fails because of missing database configuration, maccaroon secret and so on.

The goal I'd like to work towards is to get synapse to be able to come up with the minimum required information, which I gather is basically the server name and the report stats setting, because that is what is currently used to generate the default config.

Since synapse can generate all the default information into a config file itself, I think it would be nice to have synapse in charge of generating its config. Currently the Debian homeserver.yaml is slightly different from the one synapse would create via --generate-config in e.g. that the log.yaml doesn't include the server name and is called not called .conf.

These, of course, are all things that could be fixed for the Debian packaging, but I think that everybody would win if this work were not necessary (no extra work, less subtle differences between what the Debian package installs and what would be the result of a manual install, also other distros could use it for free).

(and, presumably, you want to stop the package generating /etc/matrix-synapse/conf.d/server_name.yaml etc)

I'm somewhat fine with /etc/matrix-synapse/conf.d/server_name.yaml. Not a big fan of debconf myself, but on Debian that's the mechanism of communicating stuff that a user can set during install. All in all it can fit into my goal of providing minimal info to synapse and have synapse fill in the rest that is left unspecified.

Does this sound ok?

[richvdh]

Honestly, I am not a huge fan of the output of --generate-config, and I don't think it's any better, in general, that the defaults that come in the debian package.

I think it would be nice to have synapse in charge of generating its config

It kindof is, in that https://github.com/matrix-org/synapse/blob/develop/debian/build_virtualenv#L87-L111 builds the homeserver.conf that is shipped in the debian package based on the output of --generate-config.

e.g. that the log.yaml doesn't include the server name and is called not called .conf.

Personally, I find log.yaml a more sensible name than SERVERNAME.log.config (it is a yaml file, and why do we embed the server name in it?)

I just don't really think this is a terribly useful direction, I'm afraid.

[behrmann]

Honestly, I am not a huge fan of the output of --generate-config, and I don't think it's any better, in general, that the defaults that come in the debian package.

Sure. I'm not really a partisan for which one is better. My thrust is more in the direction of not having - in my opinion - needless differences. I think the default config should look as similar as possible between all the possible ways of deploying synapse.

I think it would be nice to have synapse in charge of generating its config

It kindof is, in that https://github.com/matrix-org/synapse/blob/develop/debian/build_virtualenv#L87-L111 builds the homeserver.conf that is shipped in the debian package based on the output of --generate-config.

I think I didn't formulate that very clearly. I'd like synapse as much in charge as possible without, e.g. the intermediary of the packaging, i.e. if synapse can generate it's config from some seed information, I'd prefer not shipping it. This comes from the above-mentioned goal of having default configs look as similar as possible between different ways of deploying synapse.

Also I'd like for this to be able to happen without user interaction when possible.

e.g. that the log.yaml doesn't include the server name and is called not called .conf.

Personally, I find log.yaml a more sensible name than SERVERNAME.log.config (it is a yaml file, and why do we embed the server name in it?)

About this one I somewhat agree. I don't see a reason to embed the server name in the filename either, but again, from my point of view there's fewer reasons to call this file by default SERVERNAME.log.config on installations from source and log.yaml when installing from the package.

I do see a good point in not calling that file log.yaml, though, because with this suffix synapse could potentially confuse it for one of its config files if --config-path were pointed at the directory containing it.

I just don't really think this is a terribly useful direction, I'm afraid.

I'll close this one for now, because in the current form this can't work this way, but I'll tinker some more on the general idea. Thanks for the feedback, though! :)

[richvdh]

but again, from my point of view there's fewer reasons to call this file by default SERVERNAME.log.config on installations from source and log.yaml when installing from the package.

I'd support a PR which removed SERVERNAME from the name generated by --generate-config 😇