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.

Sign up for GitHub

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

Jump to bottom

MSC1802: Remove the '200' value from some federation responses #1802

Merged
merged 9 commits into from
Sep 10, 2019
Merged

MSC1802: Remove the '200' value from some federation responses #1802

merged 9 commits into from
Sep 10, 2019

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Jan 14, 2019 •
edited by richvdh
Loading

Rendered

Resolves #1418

@babolivier babolivier added the proposal A matrix spec change proposal label Jan 14, 2019
Half-Shot
Half-Shot approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

This seems very sensible, and definitely something I'd like to see in r0.

@erikjohnston
Copy link
Member

Cool! Though given we can do this in a backwards compatible way, we should probably do so. Basically, in the same way as #1794 we can fix this in v2 federation API, and at some point in the future deprecate the v1 API

@babolivier
Copy link
Contributor Author

Basically, in the same way as #1794 we can fix this in v2 federation API, and at some point in the future deprecate the v1 API

I didn't think about that option. I guess that's indeed better. I'll rewrite my proposal to use it rather than the current proposed solution.

@babolivier babolivier mentioned this pull request Jan 21, 2019
Closed
@babolivier
Copy link
Contributor Author

ftr this propsal is ready for review (I hadn't added the in-review label for it as I'd expect someone from the specs core team to do it)

@babolivier babolivier requested a review from turt2live May 1, 2019 15:53
turt2live
turt2live approved these changes May 1, 2019
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

seems reasonable, now it just needs more feedback from others :D

@turt2live turt2live removed the r0 P2 label Jun 3, 2019
@Half-Shot Half-Shot mentioned this pull request Jun 9, 2019
Open
@Half-Shot
Copy link
Contributor

#2102 plans to make a change so that federation endpoints expect canonical JSON rather than JSON be sent along the wire, in the interest of keeping things light for the recipient. I don't want to bring the bikeshed to this proposal, but if we are going to add a new endpoint version then it might be worth merging that rule with this proposal.

@babolivier
Copy link
Contributor Author

Thanks for the heads up @Half-Shot, I'll keep an eye on #2102 to see how it evolves (because it's not clear right now if it's going to end up in the spec).

@Half-Shot
Copy link
Contributor

Myself and @anoadragon453 came across the delight of /send* endpoints again today /s. Can we get a few more eyes on this?

@anoadragon453
Copy link
Member

Yes please.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Jul 26, 2019 •
edited by ara4n
Loading

Team member @anoadragon453 has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jul 26, 2019
@turt2live
Copy link
Member

I am assuming my past self isn't an idiot, so:

@mscbot reviewed

@babolivier
Copy link
Contributor Author

babolivier commented Sep 5, 2019 •
edited
Loading

Looking at Construct's code (which I think is the only HS implementation that I've seen used in rooms apart from Synapse), I can't see the [200, {...}] response format being used anywhere near /send so sgtm.

@babolivier babolivier changed the title MSC1802: Standardised federation response formats MSC1802: Remove the '200' value from some federation responses Sep 5, 2019
@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Sep 5, 2019
@mscbot
Copy link
Collaborator

mscbot commented Sep 5, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Sep 10, 2019
@mscbot
Copy link
Collaborator

mscbot commented Sep 10, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live merged commit c00fe02 into master Sep 10, 2019
@turt2live turt2live added this to Implementation stages in Client-server r0.6 proposals Sep 10, 2019
@behouba behouba mentioned this pull request Oct 1, 2019
Merged
@anoadragon453 anoadragon453 mentioned this pull request Oct 30, 2019
Closed
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@richvdh
Copy link
Member

richvdh commented Apr 22, 2020

This was implemented by synapse in matrix-org/synapse#6349, so I think this is just pending a spec PR?

@richvdh richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed disposition-merge finished-final-comment-period labels Apr 22, 2020
@turt2live turt2live self-assigned this May 15, 2020
turt2live added a commit that referenced this pull request May 15, 2020
@turt2live
Spec v2 send_join and send_leave endpoints
a2a1694 
MSC: #1802

Fixes #2541

This also adds the v2 invite endpoint to the ACL protected list as that appears to be an omission.
@turt2live turt2live mentioned this pull request May 15, 2020
Merged
@turt2live
Copy link
Member

Spec PR: #2547

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels May 15, 2020
@turt2live
Copy link
Member

Merged 🎉

@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels May 19, 2020
@turt2live turt2live mentioned this pull request May 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ara4n ara4n ara4n left review comments

@turt2live turt2live turt2live approved these changes

@Half-Shot Half-Shot Half-Shot approved these changes

Assignees

@turt2live turt2live

Labels
kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
No open projects
Client-server r0.6 proposals
Implementation stages
Superceded by https://github.com/orgs...
  
To Do S2S
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace the [200, {}] response for most of the federation requests
9 participants
@babolivier @erikjohnston @Half-Shot @anoadragon453 @mscbot @turt2live @richvdh @ara4n @neilisfragile