docs: Add KSCrash migration strategy document#8094
Conversation
NinjaLikesCheez
added
the
ready-to-merge
NinjaLikesCheez
requested review from
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
| @@ -0,0 +1,59 @@ | |||
| # KSCrash Migration Strategy | |||
There was a problem hiding this comment.
m: I believe this might be better suited to live in DECISIONS.md
There was a problem hiding this comment.
I agree, once a decision is made - would it be better to open this for discussion as an issue and close this out?
There was a problem hiding this comment.
I think we can keep this doc open and collapse it into a decision at the end.
|
|
||
| --- | ||
|
|
||
| ## Option B: Dual integrations on `main` (proposed) |
There was a problem hiding this comment.
My main concern with this approach is that we have to add KSCrash to the dependencies in Package.swift, and every user needs to check it out, even if they do not use it. There is no conditional compilation flag for Package.swift
There was a problem hiding this comment.
Yup, that is the main downside (one I will note in the doc as well). In testing it's a ~3.3MB checkout with SPM which seemed negligible - if you know of a better way to integrate this (until the cut over) I'm happy to not add it as a direct dependency.
|
|
||
| ## Proposal: Option B | ||
|
|
||
| The dual-integration approach is lower risk and produces a better end result. The `SDK_V10` flag already exists in the build system. |
There was a problem hiding this comment.
If we accept the downside of having KSCrash as a dependency even if we don't use it yet (gated by feature flag), I would also go with this option so we can do releases with partial functionality (if applicable) and have a full V10 testing setup on main rather than very late with a massive PR to review
|
|
||
| - Long-lived branches diverge; merge conflicts galore | ||
| - The final merge PR is large, hard to review, and risky to ship | ||
| - Other teams (React Native, Flutter, Unity) have to swap branches to test |
There was a problem hiding this comment.
I would also add, as a "con", that e2e testing requires continuous bidirectional sync, since the invariant of those tests should be that most tests should be able to run on both sides. Which is a considerable churn.
There was a problem hiding this comment.
Good point - will add that shortly
|
|
||
| **Pros** | ||
|
|
||
| - Work ships incrementally to `main`; no merge cliff & conflict mess |
There was a problem hiding this comment.
I would say "no merge cliff" or "'flip the switch' change" are only true if the KSCrash-related CI is fully integrated on main and blocks PRs, if something broke. Otherwise, this still relieves us from continuous sync efforts, but we defer the pain.
There was a problem hiding this comment.
That is true however, v10 code is already expected to be buildable by the CI and would block PRs merging. The testing side currently isn't integrated, but by the time the work is set to be delivered this should be integrated and blocking. Do you mean something else I've not considered?
There was a problem hiding this comment.
v10 code is already expected to be buildable by the CI
Sure, buildable is great, but far from "no merge cliff". For "no merge cliff", I would expect not only builds but all tests to run for v10 and block PRs.
|
|
||
| **Cons** | ||
|
|
||
| - Both integrations live in the codebase simultaneously for a period |
There was a problem hiding this comment.
I think this is not a "con" in its own right. That is just a fact of this option. However, regarding "merge cliff" etc., an essential con is that if this should really be integrated early and provide feedback, PRs for v9 might be blocked by tests that cover a rather global invariant for v10.
We could express this as a separate option "C" or an option to the option, but the crux with option B is: do we want to integrate early so we have "no merge cliff" and "no conflict mess", or are we happy with solely "no conflict mess". The former means PR blocking CI for V10, the latter means V10 being colocated on the same branch.
There was a problem hiding this comment.
Understood - I'll schedule some time this week to think about this aspect. Maybe it would be acceptable to the team to skip SentryCrash tests in v10 mode. Otherwise I'll incorporate this feedback into the document - thanks!
There was a problem hiding this comment.
Just to be clear: this doesn't have to be a biggie or even an up-front decision; it should just calibrate expectations, and our written assumptions should reflect that.
If we are interested in flipping the switch, then the test battery must run continuously. Even if v9 PRs aren't outright blocked, there should be something obviously red enough for us to act on (that is what I meant yesterday in our sync).
If we only colocate and build together to primarily reduce the churn from continuously syncing two branches, then we should be aware that we do not have feedback on whether main v9 and v10 drift on global invariants (some of which are not expressible via interfaces or types in general). And that means we must allocate some time after flipping the switch.
3 participants
📜 Description
Adds a develop-doc with the two potential migration strategies for KSCrash along with a recommendation of which approach to take.
💡 Motivation and Context
Early alignment on the strategy will allow us to start landing code changes incrementally sooner rather than later
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.#skip-changelog
Closes #8095