Skip to content
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

Use v1 and v3 endpoints for /thirdparty tests #1310

Merged
merged 3 commits into from
Apr 3, 2023
Merged

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Oct 26, 2022

Updates the endpoints used when testing /thirdparty

@S7evinK S7evinK requested a review from a team as a code owner October 26, 2022 11:30
@S7evinK S7evinK changed the title Use v1 and v3 endpoints for /thridparty tests Use v1 and v3 endpoints for /thirdparty tests Oct 26, 2022
Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but the synapse tests are failing.

Is this blocked on matrix-org/synapse#11158?

@S7evinK
Copy link
Contributor Author

S7evinK commented Nov 2, 2022

Yep, this is blocked by that issue.
Keeping unstable in Sytest and using the fallback in Dendrite unfortunately results in Destroying unresponded HTTP request to /appservs/1/_matrix/app/v1/thirdparty/protocol/ymca

@clokep
Copy link
Member

clokep commented Mar 28, 2023

I made some more changes, I think pairing this with matrix-org/synapse#15317 will fix the tests.

I also pushed this as a separate branch: https://github.com/matrix-org/sytest/pull/new/clokep/appservice-urls so that the synapse PR picks up these changes via branch matching.

@clokep
Copy link
Member

clokep commented Mar 28, 2023

See https://github.com/matrix-org/sytest/actions/runs/4545210009 run against the matching branch.

@clokep
Copy link
Member

clokep commented Mar 28, 2023

See matrix-org/sytest/actions/runs/4545210009 run against the matching branch.

This branch (and the Synapse PR) are all green for Synapse.

The PR is all green for Dendrite.

I think this is likely OK to merge?

Note that I just dropped the legacy paths altogether. I think this is OK since they were from before r0.1.0 of the application service spec. (Aka they're ancient.)

I'm going to put this back into the review queue for a double check on my logic.

@clokep clokep requested a review from a team March 28, 2023 16:38
@clokep clokep merged commit 29193f1 into develop Apr 3, 2023
@clokep clokep deleted the s7evink/thirdparty branch April 3, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants