Disable federation when using SQLite by default

[babolivier]

Fixes #2917

[codecov]

Codecov Report

Merging #5078 into develop will increase coverage by 0.01%.
The diff coverage is 60%.

@@             Coverage Diff             @@
##           develop    #5078      +/-   ##
===========================================
+ Coverage    61.72%   61.73%   +0.01%     
===========================================
  Files          335      335              
  Lines        34512    34517       +5     
  Branches      5672     5673       +1     
===========================================
+ Hits         21301    21308       +7     
+ Misses       11683    11678       -5     
- Partials      1528     1531       +3

[aaronraimist]

On one hand I like the easy enable_federation option instead of telling people to set the whitelist to nothing or their own domain to disable federation.

On the other hand I wish the name of the config option had more of the warning built in to the name of the config, maybe almost as extreme as yes_i_want_my_server_to_catch_on_fire. I feel like a lot of people don't read the comments, or they use some package that doesn't provide the full comment above the command, just a huge list of commands.

[Half-Shot]

Question for maintainers: Are we also making the packaging switch to postgres by default, otherwise we are going to be shipping a synapse that won't federate by default?

[aaronraimist]

^ is #4270 / #2317 btw

[richvdh]

So I have two main concerns with this.

Firstly, it's going to break all the existing deployments out there which use sqlite and federation and we're going to have a lot of confused users. We can try to mitigate that with warnings in the upgrade notes, but most people don't read them, and a lot will receive this as an automated update, so we'd need to put something in the debian news file too. In any case I'm not sure we should be making this sort of change with a minor release. One to discuss with the team I think.

Secondly, I don't think we can do this until we fix #4877, otherwise we're going to have even more users with broken databases.

[richvdh]

Question for maintainers: Are we also making the packaging switch to postgres by default, otherwise we are going to be shipping a synapse that won't federate by default?

I'm not too uncomfortable with shipping such a synapse - you have to do a few extra steps to get federation working correctly as it is. So I think we should treat the decision on whether to switch to postgres by default as a separate question, which as @aaronraimist says, is #4270 / #2317.

[babolivier]

Right, so I agree with the fact that it shouldn't be merged right away, especially not before we fix synapse_port_db. I suggest we just focus on getting this in shape right now then let it hang until #4877 is closed, which would then be a good time to have that discussion. wdyt?

stale

stale

[richvdh]

couple of minor nits. Still not sure what we're going to do with this PR, though. We can't land it, and I fear it will bit-rot.

[richvdh]

if it is ignored when using postgresql, surely it doesn't matter what it defaults to? I think you could remove the whole sentence about the default: it's pretty obvious what the default is from the context.

[richvdh]

spare line is spare

[richvdh]

closing for now, since it's bitrotting and not being worked on. we can reopen when work resumes.

[aaronraimist]

#4877 is fixed now

[aaronraimist]

Can we reopen this?

[richvdh]

well, it's still bitrotten. happy to reopen if it gets brought up to date.