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

sdk: Add support for authenticated media stable feature #3961

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

zecakeh
Copy link
Collaborator

@zecakeh zecakeh commented Sep 8, 2024

Was added post-merge to the MSC in matrix-org/matrix-spec-proposals#4180 for servers that support authenticated media but do not support all of Matrix 1.11 yet.

Requires a kind of hack to force Ruma to select the stable endpoint by ignoring the real versions advertised by the homeserver for this particular endpoint.

@zecakeh zecakeh requested a review from a team as a code owner September 8, 2024 09:22
@zecakeh zecakeh requested review from stefanceriu and removed request for a team September 8, 2024 09:22
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.20%. Comparing base (1eecb2d) to head (02751ca).
Report is 186 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/media.rs 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3961      +/-   ##
==========================================
+ Coverage   84.17%   84.20%   +0.03%     
==========================================
  Files         267      267              
  Lines       28181    28195      +14     
==========================================
+ Hits        23720    23742      +22     
+ Misses       4461     4453       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Was added post-merge to the MSC for servers that support
authenticated media but do not support all of Matrix 1.11 yet.

Signed-off-by: Kévin Commaille <[email protected]>
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks!

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Is it possible to assert in the tests that we are actually forcing the matrix version?

@Hywan Hywan requested review from Hywan and removed request for stefanceriu September 9, 2024 06:36
@zecakeh
Copy link
Collaborator Author

zecakeh commented Sep 9, 2024

I don't see how we can assert that directly, everything happens inside that one call to the method. It is asserted indirectly by the fact that it uses the authenticated media endpoint.

@zecakeh
Copy link
Collaborator Author

zecakeh commented Oct 8, 2024

@Hywan, maybe you can clarify what you meant, if it is blocking the review?

@Hywan
Copy link
Member

Hywan commented Oct 8, 2024

Hmm, it seems fine actually, I don't remember exactly what I was referring to. I'm approving and merging this PR.

@Hywan Hywan merged commit 2dcf06f into matrix-org:main Oct 8, 2024
40 checks passed
@zecakeh zecakeh deleted the auth-media-stable-feature branch October 8, 2024 10:22
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.

2 participants