osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

Create a State breaking label / flow for PRs

Open ValarDragon opened this issue 3 years ago • 3 comments

Background

Something thats come up a few times is we need a process for between major releases reviewing every state breaking change. This issue details some plans that help this direction, and ultimately should help in mitigating release overhead.

One big concept is minimizing the amount of "noise" to be reviewed, and the amount

Lets make every PR have one of three labels:

  • State machine breaking
  • State machine compatible
  • State machine compatible, depends on breaking change (e.g. backport conflict)

Then require every PR to have one of these labels. If compatible must get backported (and thus lowers our release-time review overhead, as these can get incrementally deployed on mainnet)

To make this feasible I think we need to be improving / building the following fronts:

  • [x] Create labels
  • [x] Require every PR to have one of these labels in order to be mergeable to main
  • [ ] Create automation to preliminarily (over-declare) things as state-machine breaking
    • [ ] e.g. if there was a change to keeper
    • [ ] Eventually we should be making some AST parsing to do this
  • [ ] Ease backporting pains
    • [ ] We should make tooling to fix the backporting issues with go version. (E.g. edit the commit it attempts to cherry-pick to replace all versions) This maybe goes away as a problem, if we ensure main is always at the same version as last release, but this is going to be annoying at release boundaries as well.
    • [x] Make CI guarantee backport state machine compatability (e.g. if label is present, attempt backport and run our v10.x bot)
    • [ ] Make backports that come through this process not require a PR if there is no merge conflict and tests pass
    • [ ] Perhaps require a backport PR to get automatically created, and merged first if state machine compatible but conflicts? (Shouldn't be happening often if process maintained well?)
  • [ ] Improve confidence in changes not being state machine breaking
    • [x] State syncing & running a mainnet node automatically (in progress)
    • [ ] Make simulator integrations to check that simulation results on old code patch are same before & after change.
  • [ ] Make tooling to synthesize the state machine break results into which messages had changes in their call stack.
    • [ ] Should be doable by making some AST parsing tooling to see every function a message server could call into, and checking if any changes. (And adding some exceptions for outliers)
    • [ ] Should be made many commit, and then give us an easy report.
  • [ ] All state machine breaking PRs, should get tracked in an issue, so we know to check them in QA

ValarDragon avatar Jul 15 '22 19:07 ValarDragon

I like the idea. I think we should also invest in documenting how to identify state-breaking changes in CONTRIBUTING.md

p0mvn avatar Jul 15 '22 19:07 p0mvn

This makes a lot of sense! Especially since we've had PRs in the past that were discovered to be state-machine breaking during review, so I think requiring a label would prevent (or at least minimize the chance of) PRs like those getting merged without the appropriate label

For the point on automation to preliminarily declare things as state-machine breaking, is there a framework we can use to determine is a PR is 100% not state-machine breaking (e.g. "if it only touches X, Y, or Z files/file types") and if so, would it be feasible to make into a CI job? Is this what you had in mind with the AST parsing point?

AlpinYukseloglu avatar Jul 15 '22 23:07 AlpinYukseloglu

I love the idea of requiring labels being added to PRs, but I'm somewhat skeptical that automation will be useful or at least, it will be non-trivial to get right.

alexanderbez avatar Jul 17 '22 05:07 alexanderbez