-
Notifications
You must be signed in to change notification settings - Fork 376
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
Upgrade Synapse to 1.19.1 #6442
Upgrade Synapse to 1.19.1 #6442
Conversation
7b74d6d
to
13c0c83
Compare
This is necessary after - raiden-network/raiden-service-bundle#187 - raiden-network/raiden#6442 With those PRs the set of federation servers can become disjunct between different Raiden client versions. Therefore we can't rely on the previous check in the Raiden client if the PFS' server is in the known servers set. The RoomID is uniquely identifying a specific broadcasting room and can therefore be used as a better information source for this check.
This is necessary after - raiden-network/raiden-service-bundle#187 - raiden-network/raiden#6442 With those PRs the set of federation servers can become disjunct between different Raiden client versions. Therefore we can't rely on the previous check in the Raiden client if the PFS' server is in the known servers set. The RoomID is uniquely identifying a specific broadcasting room and can therefore be used as a better information source for this check.
13c0c83
to
5f4e713
Compare
5f4e713
to
2412a67
Compare
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.
Looks good, I just left some random comments as usual.
if "credentials_file" in config: | ||
credentials_file = Path(config["credentials_file"]) | ||
if not credentials_file.exists(): | ||
raise AssertionError(f"Credentials file '{credentials_file}' is missing.") | ||
try: | ||
self.credentials = json.loads(credentials_file.read_text()) | ||
except (JSONDecodeError, UnicodeDecodeError, OSError) as ex: | ||
raise AssertionError( | ||
f"Could not read credentials file '{credentials_file}': {ex}" | ||
) from ex | ||
elif "admin_credentials" in config: | ||
self.credentials = config["admin_credentials"] | ||
else: | ||
raise AssertionError( | ||
"Either 'credentials_file' or 'admin_credentials' must be specified in " | ||
"auth provider config." | ||
) |
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.
Do we really need all these cases, now?
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.
This now covers both cases, the one used in the tests as well as the RSB setup.
The plan is to have this code only once and not two slightly different versions in here and in the RSB.
def check_password(self, user_id, password): | ||
async def check_password(self, user_id: str, password: str) -> bool: |
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.
Did synapse change away from twisted, or does twisted work with the async syntax, now?
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.
They are slowly converting to asyncio.
This API has already been changed.
I'm not sure what they're using as a compatibility layer or if twisted natively supports that now, but one of the twisted maintainers works for them so I assume they have that covered.
] | ||
} | ||
|
||
Which of the two lists is returned is controlled by the ``server_list_type`` argument. |
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.
We could also just return all and let the caller choose by indexing the returned dict.
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.
Yeah I thought about that, but wanted to keep the change as small as possible.
If you think it's important I'll change it.
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.
It's not. I just like to think aloud when reviewing PRs.
This upgrades the Synapse requirement to 1.19.1 and fixes all backwards incompatibilities that have been introduced. This is mainly related to the [alias semantics change (MSC2432)](matrix-org/matrix-spec-proposals#2432).
Depends-on: raiden-network/raiden-service-bundle#186 Related: raiden-network#6443, raiden-network#6212 This splits the known servers list into an 'active' and 'all' part. The 'active' part is read by the Raiden client and used to select a server to use. The 'all' part is used by the RSB as the federation whitelist. The purpose of this is to allow new servers to be added without making the Raiden client immediately try to use them while they are not yet ready.
This changes the is-pfs-on-the-same-federation check to use the new `matrix_room_id` field from the PFS info response. See raiden-network/raiden-services#858 for details
2412a67
to
e0c5b67
Compare
BTW the scenario player test can't succeed until the RSB PR is merged, which needs this PR to be merged. |
Description
This upgrades the Synapse requirement to 1.18.0 and fixes all backwards incompatibilities that have been introduced.
This is mainly related to the alias semantics change (MSC2432).
Fixes: raiden-network/raiden-service-bundle#180
Fixes: #6424
Due to this being backwards incompatible server side we need to come up with a migration strategy before merging this: #6443 [done]