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

Switch back to using Sytest dinsic branch #99

Merged
merged 2 commits into from
Jul 13, 2021
Merged

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented May 21, 2021

As part of the 1.31.0 mainline merge (#97), we updated the buildkite pipeline to more closely match mainline. Unfortunately we accidentally also updated the sytest portion verbatim, which meant we were now running the matrixdotorg/sytest-synapse:buster/testing/etc. images against synapse-dinsic, when really we want matrixdotorg/sytest-synapse:dinsic.

Additionally, and perhaps more importantly, a commit that landed on Sytest develop which reverted some dinsic-specific changes was accidentally merged into dinsic.

Both of these broke the CI as dinsic started to fall behind mainline again.

This PR specifically returns us back to the dinsic tag of matrixdotorg/sytest-synapse, as well as removes some Linux distro variations which we aren't using for dinsic. While this commit specifically fixes the Sytest portion.

matrixdotorg/sytest:dinsic and matrixdotorg/sytest-synapse:dinsic each were updated and built with Debian buster (10) for this PR: https://hub.docker.com/r/matrixdotorg/sytest-synapse/tags

As part of the 1.31.0 mainline merge, we updated the buildkite pipeline to
more closely match mainline. Unfortunately we accidentally also updated the
sytest portion verbatim, which meant we were now running Sytest develop
against dinsic. This inevitably broke as dinsic started to fall behind
mainline again.

This commit returns us back to the dinsic tag of matrixdotorg/sytest-synapse,
as well as removes some Linux distro variations which we aren't using
for dinsic.
@anoadragon453
Copy link
Member Author

The failing Sytest is DINUM-specific -> User info endpoint correctly specifies a deactivated remote user, and is failing on workers only due to the federation endpoint (/_matrix/federation/v1/query/user_info) only being registered when the CS endpoint /_matrix/client/unstable/user/<user_id>/info is registered:

class SingleUserInfoServlet(RestServlet):
"""
Deprecated and replaced by `/users/info`
GET /user/{user_id}/info HTTP/1.1
"""
PATTERNS = client_patterns("/user/(?P<user_id>[^/]*)/info$")
def __init__(self, hs):
super(SingleUserInfoServlet, self).__init__()
self.hs = hs
self.auth = hs.get_auth()
self.store = hs.get_datastore()
self.transport_layer = hs.get_federation_transport_client()
registry = hs.get_federation_registry()
if not registry.query_handlers.get("user_info"):
registry.register_query_handler("user_info", self._on_federation_query)

That CS endpoint (and thus the federation query handler) is only registered when we have a client resource handler, which of course isn't set up for the federation_reader in SyTest.

The other bit of all this is that this user/<user_id>/info endpoint has been deprecated for users/info, which allows for checking the info of multiple users at once. This endpoint doesn't suffer from needing a client resource. Therefore my suggestion would be to just drop the deprecated endpoint and update the tests to use the new one -- assuming Tchap clients don't use GET /_matrix/client/unstable/user/<user_id>/info anymore, and instead now use POST /_matrix/client/unstable/users/info. @giomfo can you confirm if that's the case?

@giomfo
Copy link
Member

giomfo commented Jun 25, 2021

Tchap clients don't use GET /_matrix/client/unstable/user/<user_id>/info anymore, and instead now use POST /_matrix/client/unstable/users/info.

@anoadragon453 : yes, I confirm

@babolivier
Copy link
Contributor

babolivier commented Jul 12, 2021

I'm tempted to move those tests to Synapse's suite rather than Sytest, since I'm not sure it should even live in Sytest (and arguably the whole /info shenanigans should live in a module). @anoadragon453 do you see any reason why it should stay in Sytest?

For the record, these tests were added in matrix-org/sytest#741

@anoadragon453
Copy link
Member Author

Yeah this is in no way a part of the spec so it should live somewhere in Synapse's domain instead of Sytest. And agreed on the module.

@babolivier
Copy link
Contributor

Alright, I think it'll be easier to just shove it into a module directly, so I'll do that. It will require a change to the URL (since modules can't register paths under /_matrix/client) but it should be fine as long as we keep the old endpoint around for a bit to allow for clients to update, since it's just reading from the database (so there should be no difference between calling the old and new endpoint).

@anoadragon453
Copy link
Member Author

@babolivier I might advise just modifying the tests for now (or even disabling them as they're testing an endpoint nobody uses anymore) to unblock other work before starting on a new module?

@babolivier
Copy link
Contributor

@anoadragon453 Yes, my plan is to just remove them from Sytest for now to unblock #100, I was mostly considering what should be done after that because I don't want too much time to pass without having any test around this feature, so I'd rather have a plan from the start.

@babolivier
Copy link
Contributor

babolivier commented Jul 13, 2021

I've removed the tests in matrix-org/sytest#1078. I have now rebuilt and pushed the dinsic docker images for sytest, so I'm going to kick the failing sytest jobs again and merge this if they pass.

@babolivier babolivier merged commit fcc10d9 into dinsic Jul 13, 2021
@anoadragon453 anoadragon453 deleted the anoa/fix_pipeline branch July 13, 2021 16:44
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.

3 participants