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

Stable Releases: Definition and Process #6394

Merged
merged 52 commits into from
Jul 14, 2020
Merged

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jun 10, 2020

Related to #6522

closes #1728


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@alessio alessio changed the title Stable Release Update: Definition and Process Stable Release Updates: Definition and Process Jun 10, 2020
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

@alessio thanks for putting time into this, but I think it would benefit us all if the PR body at least contains some motivation or rationale for these changes. I left a comment, but the gist is I really don't think the change is nomenclature is warranted.

CONTRIBUTING.md Outdated
@@ -228,32 +229,40 @@ only pull requests targeted directly against master.
- Tag the release (use `git tag -a`) and create a release in Github
- Delete the `RC` branches

### Point Release Procedure
Copy link
Contributor

Choose a reason for hiding this comment

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

I would really prefer to keep the nomenclature point releases because that is what they are. In fact, in very rare cases, they include breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing a breaking change when it's necessary to fix a bug is IMHO not a problem and would be a good rationale to grant an SRU exception. I'll clarify this in the What qualifies as SRU

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
and PRs are merged into `master`, if a contributor wishes the PR to be released as SRU into the
`v0.38.N` point release, the contributor must:

1. Add `0.38-sru` label
Copy link
Member

Choose a reason for hiding this comment

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

i would prefer to keep backport this is another thing that we will have to update on every major release. This increases the maintenance load and cognitive load of everything that needs to be updated in these scenarios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, so instead of backport I'd propose to use a semantic versioning-like nomenclature: MAJOR.MINOR.PATCH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, regardless of the name (we agreed that we stick with backport), I think that such living branch should carry the next version number to 1. avoid versioning mistakes 2. make clear to users what changes are going to be in exactly which release. Wdyt? @marbar3778 @alexanderbez

Copy link
Member

Choose a reason for hiding this comment

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

yes agree with you. 0.38-backport works for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually meant "it should include the patch number" :) e.g. 0.38.5-backport

Copy link
Contributor

Choose a reason for hiding this comment

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

The proposal of 0.38.5-backport looks fine to me. I am infavor of calling it backport and not changing historical nomenclature.

@alessio Can you update this file accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Please check

CONTRIBUTING.md Outdated Show resolved Hide resolved
@alessio
Copy link
Contributor Author

alessio commented Jun 11, 2020

@alexanderbez Rationale can be found here:

https://github.com/cosmos/cosmos-sdk/pull/6394/files#diff-6a3371457528722a734f3c51d9238c13R281

Rationale

Unlike in-development master branch snapshots, Cosmos-SDK releases are subject to much wider adoption,
and by a significantly different demographic of users. During development, changes in the master branch
affect SDK users, application developers, early adopters, and other advanced users that elect to use
unstable experimental software at their own risk.
Conversely, users of a stable release expect a high degree of stability. They build their applications on it, and the
problems they experience with it could be potentially highly disruptive to their projects.
Stable release updates are recommended to the vast majority of developers, and so it is crucial to treat them
with great caution. Hence, when updates are proposed, they must be accompanied by a strong rationale and present
a low risk of regressions, i.e. even one-line changes could cause unexpected regressions due to side effects or
poorly tested code. We never assume that any change, no matter how little or non-intrusive, is completely exempt
of regression risks.
Therefore, the requirements for stable changesets are different than those that are candidate to be merged in
the master branch. When preparing future major releases, our aim to design the most elegant, user-friendly and
maintainable SDK possible often entails fundamental changes to the SDK's architecture design, rearranging and/or
renaming packages, reducing code duplication so that we maintain common functions and data structures in one
place rather than leaving them scattered all over the code base. However, once a release is published, the
priority is to minimise the risk caused by changes not strictly required to fix qualifying bugs; this tends to
be correlated with minimising the size of such changes. As such, the same bug may need to be fixed in different
ways in stable releases and master branch.

@alessio
Copy link
Contributor Author

alessio commented Jun 11, 2020

In release management, backport generally means something different than Stable Release Update. A Stable Release Update is a bugfix-only change. A backport refers usually to a new feature available in a successive release that by the time the stable release was published had not yet been implemented

@tac0turtle
Copy link
Member

tac0turtle commented Jun 11, 2020

My preference would be to stick to something everyone knows. Backporting is currently used & understood.

We recommend using releases instead of master anyways so I'm not sure what benefits this brings. We don't specify that backporting or SRU's will not entail new features. If it's not breaking then I don't see the rationale of not including it in a patch release? Maybe instead of renaming the process, we create a rubric on what is accepted breakage and what is not, what should be backported and what should not. Currently, it is informal in what should be backported and what shouldn't.

It would be nice to keep tendermint, sdk & gaia all on the same process and naming conventions. Currently, they all use Backport terminology and I don't think they would want change.

@alessio
Copy link
Contributor Author

alessio commented Jun 11, 2020

My preference would be to stick to something everyone knows. Backporting is currently used & understood.
It would be nice to keep tendermint, sdk & gaia all on the same process and naming conventions.

No problem with that.

We don't specify that backporting or SRU's will not entail new features. If it's not breaking then I don't see the rationale of not including it in a patch release?
[...]
what is accepted breakage and what is not

It's mostly about regression risk and stability guarantees. Generally, stable release users don't want to:

  1. rewrite their code
  2. experience regressions

What qualifies as SRU and What does not qualify as SRU list some criteria to evaluate backport candidates.

@alessio
Copy link
Contributor Author

alessio commented Jun 11, 2020

@marbar3778 dixit:

it is informal in what should be backported and what shouldn't.

Correct, and the objective of this PR is to clarify what should be backported and what should not.

@alexanderbez
Copy link
Contributor

Thanks @alessio, sorry I missed that. I totally agree with the context, but that's not a rationale to change the nomenclature. I would keep what you have but just keep Point Release. This is what is used in many projects and I'd rather not introduce new verbiage.

@alessio
Copy link
Contributor Author

alessio commented Jun 11, 2020

@alexanderbez dixit:

I would keep what you have but just keep Point Release

I'm totally fine with that. I'll update the naming accordingly then

@alessio alessio added the WIP label Jun 11, 2020
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@alessio alessio requested a review from zmanian June 22, 2020 08:47
@alessio
Copy link
Contributor Author

alessio commented Jun 22, 2020

I've updated the PR - considering that all the work that @alexanderbez and @erikgrinaker are doing in preparation for 0.38.5 will likely introduce breaking changes, maybe this could be enforced once 0.38.5 is released? Thoughts?

CONTRIBUTING.md Outdated
Once a Cosmos-SDK release has been completed and published, updates for it are released under certain circumstances
and must follow the *Stable Release Update* (or SRU) procedure.

### Rationale
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marbar3778 @alexanderbez here is the rationale. Thoughts? Does that make sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

thank you for the rationale. I do agree but still dont understand the naming change. Every release is a stable release or should be. We can keep the rationale because it will help but there isnt a need for a rename IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by every release? A major.minor is a major stable release (e.g. 0.38), backports/bugfixes/updates go into point releases. Am I wrong?

3. **Stable Release Managers** will review and discuss the PR. Once consensus surrounding the rationale has been reached and the technical review has successfully concluded, the pull request will be merged in the respective `release/launchpad/0.39.X` branch, being `X` the Launchpad's next point-release and the PR included in the `0.39.X` respective milestone.

### Stable Release Exception - Bug template

Copy link
Member

Choose a reason for hiding this comment

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

This should be a github issue template. putting it here makes it harder to follow a template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be convenient. The thing is that this template is meant to be used mainly in comments on bugs that already exist and have been fixed in master that someone wants to get fixed in stable releases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2. Add a comment to the issue and ensure it contains the following information (see the bug template below):

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe good to just add this to the bug template in a way that can be easily excluded if it's not a stable release exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds doable to me, yes.

@ethanfrey
Copy link
Contributor

ethanfrey commented Jul 7, 2020

I reviewed the recent changes. It looks very nice and clear process.

I agree we are discussing this with launchpad in particular, and this is the major visible test case of this. But I see this same process (both the doc and the human learnings) to be useful for a future upgrade when we freeze another LTS release.

Edit: after reading the below post, I understand the diff and mean SRU

@alessio
Copy link
Contributor Author

alessio commented Jul 7, 2020

Thanks @ethanfrey. I'd avoid using the acronym LTS though. LTS typically means long-term as in "for a few years". That's not our plan at this stage AFAIU. We could always introduce an extended support if the need arises and we have bandwidth for it.

Copy link
Contributor

@clevinson clevinson left a comment

Choose a reason for hiding this comment

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

lgtm!

@ethanfrey
Copy link
Contributor

Looking good to me. I just saw another github notification and was curious what is blocking this from being merged? Are there any open questions/concerns?

STABLE_RELEASES.md Outdated Show resolved Hide resolved
STABLE_RELEASES.md Outdated Show resolved Hide resolved
STABLE_RELEASES.md Outdated Show resolved Hide resolved
Alessio Treglia and others added 3 commits July 13, 2020 23:35
Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Federico Kunze <[email protected]>
Copy link
Contributor

@okwme okwme left a comment

Choose a reason for hiding this comment

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

LGTM

@alessio alessio merged commit 839ee4f into master Jul 14, 2020
@alessio alessio deleted the alessio-sru-and-procedure branch July 14, 2020 08:28
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.

Document what constitutes a breaking change
7 participants