Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bring auto-accept invite logic into Synapse #17147
Bring auto-accept invite logic into Synapse #17147
Changes from 13 commits
3353f4c
3be3fb7
f612271
657baec
f82358d
0906a34
0bdcf0d
a50d304
c82e16e
9698bf4
f07a112
6db79fc
396909f
821a74b
aa6e20e
8874e58
41c851c
d038118
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally only out-of-tree modules will use the Module API, so it feels a bit odd to me to see in-tree code making use of it. But then again, it allows the code to be self-contained, and easy to extract to a third-party module if we ever wanted to do so in the future.
I wonder if instead of having an explicit config section for this module, we instead just have it installed by default into your venv. Then, just like a third-party module, a sysadmin would just configure it under
modules
as if it were installed separately.This cuts down on the number of config sections, specialised code in the module config loader, and makes migrating this code to an out-of-tree module even easier if desired.
We would just need to be careful not to integrate the code too heavily, thus making it difficult to unpick later. One way to encourage this would be to put this code under a separate directory, say
/synapse/modules
, and tests under/tests/modules
. We can then treat code in those directories as separate, intended to interact with the rest of Synapse only through the Module API, as an external module would.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the general idea.
Specifically, would we create a separate pyproject.toml file for each module? (ie. in
/synapse/modules/my_module/
)And how would we go about versioning these modules?
If versioning them, would we need to remember both to update the module version itself, as well as the overall synapse dependency version?
When installing them, would we just add them to the main pyproject.toml as a path dependency? Would this be enough to ensure they are installed in each of the various docker containers, deb package, local install, etc.?
Hopefully this makes sense to you. I ran into these things while trying this out.
These will need to be sorted out if this is to be a viable long term path forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would heavily encourage that they're not separate projects, as you lose a bunch of benefits of it being in tree (e.g. being able to use private APIs etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, that is one of the downsides. While this code doesn't actually need any private APIs, it is inevitably handy.
I can actually add another downside. While we wouldn't end up adding to Synapse's config if we made this a module... it would beg the question of how we'd actually document the config of this module. We wouldn't be able to put it in https://element-hq.github.io/synapse/latest/usage/configuration/config_documentation.html, unless we added a new section for each in-tree module... and then you've ended up making the user-visible config larger anyhow.
That leads me to think that the only benefit of keeping the modules separate would be if we ever wanted to move them out-of-tree again in future. But I think the times we'll actually do that are minimal. And if we really need to do so, then untangling it from deep within Synapse isn't impossible, just slightly more fiddly.
The initial reason for me suggesting that we keep this code separate is that internal code using the module API felt weird. But after reflection I don't think it's really an issue. It doesn't block us from modifying the API since the code is internal and can change. I also don't believe we have any assumptions in the code that all consumers of the API are external.
So all in all, I'm OK with leaving this code how it is and where it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not go in #17166 instead? How come it was moved back to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha because when I put it in that PR, the linter complained about an unnecessary ignore....
So rather than fight a circle of nonsense, I feel like it's easier just to keep it here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit cautious about introducing more
ignore
for types without trying to spend some time in understanding what has happened. By the looks of it the type inference has failed somehow, so am wondering if we've managed to introduce a loop in the typing maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this parcitular lint is related to this issue: matrix-org/synapse#11165
Which seems like quite the can of worms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The particular error is:
synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For further reference, this is the only instance of using
self._media_repo.store
outside of the media_repo source.That probably has something to do with the linter error only showing up here.
Since this code is completely unrelated to the changes in this PR, I suggest we allow the addition of the
ignore
here until the larger issue of #11165 is brought to a resolution. Adding anignore
keeps things in effectively the same state as before since mypy should have been treating this as an error, but it was silently ignoring it. At least with theignore
added, now the issue is explicit and visible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type]
this error sounds like one where we could hopefully add a type annotation instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type is generic & is specified at the following locations:
synapse/synapse/app/homeserver.py
Line 84 in 68dca80
synapse/synapse/app/generic_worker.py
Line 166 in 68dca80
synapse/synapse/app/admin_cmd.py
Line 113 in 68dca80
All of which also have an explicit type ignore on them.
I can't seem to come up with any annotation that would make mypy happy - maybe someone else knows a way, but this seems like a much deeper issue around DataStore typing.