[RFC] How should we handle backwards compatible schema changes? #60
Replies: 7 comments 5 replies
-
|
(I'll post a few top-level comments to split up discussion chains) In a general context, I think there's some value to gain from defining what constitutes the actual api surface that the schema represents, as with a concrete definition of that, it'll be easier to make decisions on what is/isn't breaking (or is acceptably-breaking). For the sake of other comments I'll be posting, I'm operating on the assumption that the API surface is the shape of the structs defined by the schema - i.e. for the (fabricated) example schema: name: Action
displayField: Name
fields:
- name: Name
- name: UnlockLink
type: link
targets: [Foo, Bar]
- name: Unknown1I'd consider the following to be API surface:
interested in thoughts on that front. |
Beta Was this translation helpful? Give feedback.
-
|
Within BM consumption; technically any change at any time is A-OK by me - BM can use arbitrary git refs, so consumers pinning schema (which they should be (glares at some consumers)) are unaffected by exds upstream changes. For a typical-consumer that is using a snapshot of the schema, I'd say
|
Beta Was this translation helpful? Give feedback.
-
|
Following through on wild deprecation ideas not being very viable long term.
on the latter, hm. a top-level optional using my prior example, name: Example
fields:
- name: Unknown0
- name: Unknown1
- name: ExistingKnown
- name: Unknown3
pending:
name: Example
fields:
- name: SomeArray
type: array
count: 2
- name: ExistingKnown
- name: NewNamethat is a bit of additional work, let's see
I think the biggest footgun there is that changes outside |
Beta Was this translation helpful? Give feedback.
-
|
Being unable to rename named fields - and with note from the other comment chain, I feel this potentially extends to As you note, there's PRs that need to be held off for renames, and such - which feeds into
with ACK that this is additional work - and hence possibly something to look into on the long term rather than expressly for 7.2, being able to eagerly merge new names under a new key during a patch cycle i feel would help reduce both of the above. i.e. - name: Recast100ms
+ pendingNames:
+ - BetterName
+ - MuchBetterNamewould produce an unchanged struct Action {
#[deprecated] Recast100ms: u64,
#[deprecated] BetterName: u64,
MuchBetterName: u64
}Come a patch window, promoting the most recent name to |
Beta Was this translation helpful? Give feedback.
-
|
Not sure how viable this one is, especially as purely Unknown sheets (happens when a new sheet is added). For example, how would we deal with unknowns being combined into an array? It might be best to do this out-of-band by relying on git histories, but I don't like the idea of that too much. |
Beta Was this translation helpful? Give feedback.
-
|
Right, no, I'd only considered names, not structural changes. Structural changes would render the above unusable. (train of thought follows) Handling structural changes would likely necessitate a new top level field, i.e. name: Example
fields:
- name: Unknown0
- name: Unknown1
- name: ExistingKnown
- name: Unknown3
+ pending:
+ - offset: 0
+ name: SomeArray
+ type: array
+ count: 2
+ - offset: 3
+ name: NewNamebut that feels really really brittle with the offsets, as it would force a bugfix in That immediate problem could be avoided via names instead, i.e. pending:
- replaces: [Unknown0, Unknown1]
name: ...but that falls apart the moment there's a reordering, i.e. fields:
- name: Unknown0
+ - name: ExistingKnown
- name: Unknown1
- - name: ExistingKnownwhich would break the array. 😩 admittedly i don't see a strong solution without making the schemas more programattic than configuration. that's certainly possible, and i'd like to think about that more, but given it's not something i personally actually benefit from (bm effectively takes the out-of-band git history approach already), i'm probably not the best driver at this immediate point in time. |
Beta Was this translation helpful? Give feedback.
-
I'm currently considering the following in non-breaking changes:
Additionally, I'm considering adding the following to the .yml schema:
My current questions:
Let me know if I missed anything. I'll make another comment below to discuss how I plan to handle actually going through with applying breaking changes. |
Beta Was this translation helpful? Give feedback.
-
|
current considerations make sense, with note ref. pending/deprecated in qs
hm, depends how the schema documents the structure i think. i've historically thought of links as effectively an enum - and under that model, while adding is okay as long as it's considered non-exhaustive, removing is always breaking. but it would be valid to document it as simply an arbitrary array - this adds an onus on consumers to be able to resolve a link by string sheet name (rather than generated static type references), but that isn't incompatible with what's possible.
personal 2c: absolutely fine - i don't really view it as "list of targets" vs "switch case", but rather "list of targets" vs "list of targets with conditions" - my current impl just models this as an optional condition on a reference.
i'd say yes, should be consistent and use pending for both. means that trivial consumers don't get breaking changes, and involved consumers can opt-in as useful.
a field can only realistically have one most-up-to-date "canonical" name, so i'm not convinced this is a problem. the stability guarantee is on the existing (deprecated) name, not the pending one waiting for the next breaking window update. if storing the history of pending names is valued, can use |
Beta Was this translation helpful? Give feedback.
-
|
Sounds good. Seems like these are the following changes I'll make:
|
Beta Was this translation helpful? Give feedback.
-
To recap how the repository layout will look after I go through with these changes:Branches:
File Layout:
Question: Should contributors send PRs to main (and then let CI/CI take those changes and upstream them to the other branches) or should it be the other way?Handling Breaking ChangesEvery month (every other month?), I'll manually fire off a github workflow to apply the following changes on all versions and branches:
Question: How often should this breaking change happen? It'll always be possible to pin to a specific git commit, so I don't think it's a bad idea to make it every month.CI/CD ToolingI'm currently building the following tooling to reduce as much manual labor and possible mistakes:
|
Beta Was this translation helpful? Give feedback.
-
|
Here's what the repository layout will look like upon the release of 7.2: Checklist of things before I'm ready for 7.2:
TODO for 7.3:
|
Beta Was this translation helpful? Give feedback.
-
|
Closing this in favor of #93. Breaking changes may not be necessary anymore (?) |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
My current plan is the following:
Unknown{some monotonically strictly increasing number}.If anyone has any suggestions or comments about this plan, please let me know. As soon as we reach a consensus here (I'll set the hard time limit to a week prior to 7.2), I'll start merging PRs.
Beta Was this translation helpful? Give feedback.
All reactions