ibc-rs icon indicating copy to clipboard operation
ibc-rs copied to clipboard

Investigate using gitflow or alternatives

Open plafer opened this issue 3 years ago • 8 comments

The main problem to fix is to be able to easily release hotfixes to previously released versions. This is currently hard and ad-hoc with everything being in main.

plafer avatar Oct 25 '22 14:10 plafer

Related: #284

plafer avatar Dec 06 '22 17:12 plafer

I think I'm missing something -- why does #284 need to be a hotfix to a previously released version, rather than just using the main branch? It seems like much more work for kind of unclear benefit.

hdevalence avatar Dec 06 '22 18:12 hdevalence

Hmm, I should amend the description. We don't always want to release whatever's in main (at least how we do things today); sometimes multiple PRs need to be merged as a logical unit (e.g. the PRs that address #221). So what I do today when there's a request for a release is to make a branch at a past commit, and make the release from there (e.g. the v0.23.x branch is used for the v0.23.x releases).

However we never carefully thought about how we want to manage branches/releases; this issue is meant to remind us to do that.

plafer avatar Dec 06 '22 18:12 plafer

Our requirements are:

  1. When a security patch or a new version of ibc-rs with an updated dependency (e.g. tendermint-rs) needs to be released, we want to be able to cut the new release with no delay
  2. Big features in development should not prevent us from releasing minor improvements whenever we want, as is the case today (see my previous comment).
  3. (Preferable) it should be easy to release a fix to multiple versions; not just the latest (e.g. release it to 1.1.x and 1.2.x).

In order to achieve those, I suggest we start using trunk-based development. The TL;DR is that everyone commits everything to main frequently (after code review), and main is always ready to be released. Specifically,

  • We commit "small PRs" directly to main regularly (no long-lived branch). Hence, most PRs will contain unfinished work. That is okay; features in development would be placed behind feature flags. The PR that completes a feature removes the feature flag (unless that feature contains a breaking change - more details below).
    • I think it's best if we relax the requirement that each PR must close an issue. It's probably best to just mention which issue the PR works towards.
    • Our CI will not test all combination of feature flags; we only test no features, and all features (as we do today I believe).
  • Breaking changes are also put behind feature flags (without the need to increment the major version). When we're ready to increment the major version, we can remove the corresponding feature flags.
  • To do a release, cut a new "release branch" (e.g. 0.25.x) and do roughly what we were already doing (except we keep the release branch around). Fixes are always commited to main and cherry-picked back on the release branch if possible. If main diverged too much from 0.25.x for that to work, reimplement the fix on the release branch. Note that this is only needed for release branches that we support; we can adapt our policy of how long a release branch is maintained based on user needs.

@romac @hu55a1n1 thoughts? If we agree that this is the way to go, then I'll submit a PR that updates CONTRIBUTING.md with this information.

plafer avatar Dec 12 '22 22:12 plafer

Related discussion on the hermes repo -> https://github.com/informalsystems/hermes/discussions/1508

hu55a1n1 avatar Dec 13 '22 15:12 hu55a1n1

As an onlooker, I'd recommend against using feature flags. They're quite a lot of work to build and maintain, and they bring a high risk of undetected breakage, because cargo's unification of feature flags means they can perform "spooky action at a distance". I also don't think they'll solve your problems, because one common type of change to make is a change to an existing API or data structure, and that's not possible to work around without essentially duplicating the entire structure that was changed -- which then means propagating that duplication through the rest of the codebase.

As a downstream API consumer, I'd also prefer if ibc-rs avoided use of feature flags, because of the attendant complexity. It would be much better to just make frequent releases and increment the semver version, allowing downstream consumers to upgrade on their own schedule, staying up-to-date with API changes as they happen.

hdevalence avatar Dec 15 '22 19:12 hdevalence

  1. (Preferable) it should be easy to release a fix to multiple versions; not just the latest (e.g. release it to 1.1.x and 1.2.x).

Is it also a requirement that releases containing critical bugfixes must be easy to update to (i.e. shouldn't contain breaking/major API changes)?

I also think we should clarify whether or not we would like to maintain long-term-support (LTS) versions which will receive backports of hot/bugfixes. I presume we wouldn't because we're still in the initial development phase (pre-v1.0.0) and breaking changes are to be expected. I think we should communicate this with our users.

I like the idea to use TBD (trunk-based development). IIUC, the idea is also to have frequent releases to keep the drift between latest release and master/main small. So ideally, we don't have to release hotfixes at all.

I agree that feature flags can be problematic. I think there's a tradeoff between ease of release & patching (git flow style) v/s quick feedback & easy merging (continuous delivery style). Might be easiest to have frequent releases (like in TBD) but without releasing incomplete work.

Would also be nice to add CI checks to build basecoin-rs, Nomic, Namada and Penumbra chains using patch git dep (PR branch) for ibc-rs to be notified as soon as we break their builds.

hu55a1n1 avatar Dec 19 '22 20:12 hu55a1n1