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

Require migration plan in MSCs #1650

Closed
wants to merge 1 commit into from

Conversation

julioromano
Copy link

@julioromano julioromano commented Sep 28, 2023

In order to make migrating software to new versions of the spec easier and less ambiguous.

Preview: https://pr1650--matrix-spec-previews.netlify.app

In order to make migrating software to new versions of the spec easier and less ambiguous.
@julioromano julioromano requested a review from a team as a code owner September 28, 2023 14:05
Comment on lines +188 to +196
- Proposals must include a migration plan for how to switch
from unstable, prefixed implementations to stable, unprefixed ones
in both client and server applications.
Such plan should include specifics like:
- How to handle immutable data (e.g. past unstable events) that can't
be migrated.
- How to switch from unstable to stable push rules.
- When (not a precise point in time, but rather after what event) clients
are allowed to stop sending unstable events.
Copy link
Member

Choose a reason for hiding this comment

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

not all MSCs even use unstable identifiers, so I think this needs some more qualification.

It also feels odd to include this requirement before introducing the concept of unstable identifiers, which doesn't happen until way down here

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, this can be made better, though perhaps this should be addressed first.

@@ -185,6 +185,15 @@ is as follows:
- Take care in creating your proposal. Specify your intended
changes, and give reasoning to back them up. Changes without
justification will likely be poorly received by the community.
- Proposals must include a migration plan for how to switch
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like the responsibility of the spec process. Clients are deliberately welcome to decide if and how they migrate, and the general case exists if the sct needs to push back on an implementation specifically.

Copy link
Author

@julioromano julioromano Sep 28, 2023

Choose a reason for hiding this comment

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

I'd generally agree with this comment. Though sometimes things can get very entangled and complicated when you have multiple MSCs depending on each other (e.g. polls, push rules, extensible events).
So the aim here was for the MSC to include some broad indication about what should and should not be sent/read by clients/servers at certain points in time.

A good example for what I mean by "certain points in time" is what's at https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/3827-space-explore.md#unstable-prefix which defines these 3 points in time as:

  • While the MSC is unstable
  • Once the MSC is merged but not in a spec version
  • Once the MSC is in a spec version

Would requiring MSCs to include such sections be something the SCT could consider?

Copy link
Member

Choose a reason for hiding this comment

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

Those sections I think are useful and represent a good MSC, though the specific details of a migration would be best left out of scope. MSC3827 does a great job of explaining what is available in the different stages without describing a migration process itself.

Copy link
Member

Choose a reason for hiding this comment

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

all the proposal here is intending to do is to force MSC writers to help set the precedent for how their MSC should be adopted by implementations once merged. Should unstable stuff be ignored? grandfathered? until when? what about new events that use the prefix? should it be coupled to room versions? or spec releases? or a timestamp?

Without this coordination, implementations are just guessing how long they need to keep grandfathered support around for; I think it's a no-brainer to ask authors to provide some guidance in the MSC.

Copy link
Member

Choose a reason for hiding this comment

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

I strongly believe that's an implementation concern, not a spec concern. Implementations don't need the MSC to coordinate migration for them, but the MSC does need to make tools available for migration to happen.

For things coupled to a room version, this is an already well-established process that should be documented outside of the MSC process anyways. We can provide similar templates for other popular forms of migration, but that should not be in the MSC itself.

Copy link
Member

Choose a reason for hiding this comment

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

I'd argue that the pre-FCP review process is meant to acquire opinion from implementers on the migration path. It's true we've done a bad job helping folks in the past, but I don't think the solution is to make the process involve implementation details.

Right, but IMHO there's no use acquiring that opinion unless it gets written down (not least because I have goldfish-brain). And if we're going to get it written down, where better than in the MSC.

I'm fully supportive of the language that MSC3827 demonstrates - we should be encouraging that level of detail, not a step further into migration planning specifically.

I think that the level of detail in MSC3287 is appropriate for MSC3287. I think that for some MSCs, that would be way too much, and for others (notably some of the polls stuff), it might not be enough.

We should also be making the MSC process more approachable so folks can feel like they can leave migration-related feedback well before FCP.

Sure. Doesn't this point towards including the migration plan in the MSC though?

Copy link
Member

Choose a reason for hiding this comment

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

I think we might all be basically on the same page but talking past each other about what exactly we have in mind. Maybe it would be interesting to think about what an appropriate level of detail would be for a migration plan for something like polls or threads. That threads doc in particular has a lot of information that wouldn't be appropriate in an MSC, like dates, and details of deployments to specific homeservers, but there are some interesting points like "Updating the client prefix will cause old threaded messages to move to the main timeline."

Copy link
Member

Choose a reason for hiding this comment

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

I'm very worried about the word "migration" here, I think. Having detail about what unstable prefixes to use is important, but dates and homeserver details are indeed out of scope in the MSC. Implementation teams should build these docs though so they are on the same page about what is important to them. For example, if unstable polls will continue being supported/visible after X release.

Copy link
Author

Choose a reason for hiding this comment

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

The more I ask around and read about this topic the more I feel this is not about knowledge or information missing from the spec or from the MSCs, but rather that this information is scattered into several places and hard to come by and make a general sense of for newcomer devs (and maybe also for some more experienced ones) which, by the nature of their job, must balance the time they spend delving into the multiply versioned and interdependent MSCs jungle vs the time they spend implementing them.

In my case (polls) the information retrieval was hindered by mainly:

  1. There are 2 versions of MSC3381 but going to https://github.com/matrix-org/matrix-spec-proposals/pulls/3381 only shows the most recent one (thankfully a link to v1 was in the issue description).
  2. MSC3381 depends on MSC1767 so a lot of the migration information was in the latter one, but I naively assumed that to implement MSC3381 it would be sufficient to have a good understanding of MSC3381 alone.
  3. While reading about MSC1767 I was told there were also two versions of it but this time there was no "link to v1" in the issue description so it was hard to both realize there was a v1 (what if nobody ever told me?) and to know how to access it, and it does not end here: there is also a third, older, version in the form of MSC1225 (this was mentioned in the MSC1767 issue description though). So at first I wasn't sure whether the term "v1" was referring to MSC1225 or if it was referring still to MSC1767 but a previous commit of it (it turned out it was the latter).
    This all made it more difficult to grasp which version of MSC3381 was tied with which version of extensible events.
  4. Even though MSC1767 explains that the e.e. migration will be tied to a room version, I failed to understand that room upgrades don't migrate existing events but actually close the room and create a new one so I couldn't come up with a sound mental model of how to tackle/prepare for the migration.

Yeah it's true that learning is a chaotic process but it's also true that what happened to me may happen to others, so is it worth spending time thinking about how to make the body of information contained in the spec and MSCs more accessible to non-SCT members?
Can adding a more structured "Unstable prefix" section to MSCs (a la MSC3827) help in this regard?

Copy link
Member

@turt2live turt2live Oct 4, 2023

Choose a reason for hiding this comment

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

Polls (and everything building on MSC1767) is a bit of a special case, and particularly awful to work in, sorry. I don't think anyone is disagreeing that "Unstable prefix" can be more structured: it's the words which go into that section which are the concern. There's some things that, at least in my opinion, are not suitable for the spec process and should not go into here (outlined above).

@richvdh
Copy link
Member

richvdh commented Feb 27, 2024

This PR seems to have become quagmired.

Personally speaking, I think that having some sort of migration plan in an MSC is often a good idea (matrix-org/matrix-spec-proposals#2867 is another example I happened to come across today which could maybe do with a more explicit plan for the migration). There is, however, validity to @turt2live's point that those are largely implementation concern, and requiring it for all MSCs is probably too strong.

@julioromano unless you're keen to continue pushing this I'm minded to close it, but thank you for your efforts to improve things here! 💚

@julioromano
Copy link
Author

@julioromano unless you're keen to continue pushing this I'm minded to close it, but thank you for your efforts to improve things here! 💚

Roger that. Indeed I won't be able to spend more time on this right now.

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