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

Release Process #2490

Merged
merged 50 commits into from
Jan 26, 2024
Merged

Release Process #2490

merged 50 commits into from
Jan 26, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Nov 24, 2023

Writing down the processes to do our releases.

Status: please review & approve so we can go ahead.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Thank you!

Have done a first pass and left some comments. IMO, we should make the process less depended on posting something into some internal channels. This would be totally opaque to the outside and not that good.

Otherwise it already is already okay!

One thing we should think about is to name things which are "unstable" internal api. Aka things that can be merged to master faster even when it requires a major version bump. I'm thinking about the Polkadot node internals that are not really exposed.

docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated
1. [ ] Check out the latest commit of `stable`.
2. [ ] Verify all CI checks of that commit.
3. [ ] Announce that commit as cutoff *Mainline* for a release in the General<sup>2</sup> chat.
4. [ ] Bump the semver of all crates <!-- FAIL-CI: We need some better process here on how to do it exactly -->
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 really not done. IMO we should bump the crate versions as we merge stuff to unstable.

I had written here some things: #2366

I mean I know that is not a perfect thing, but probably the best we can do. IMO this should not be done by a single person or after the fact as this is otherwise too hard to follow.

Copy link
Member Author

@ggwpez ggwpez Nov 24, 2023

Choose a reason for hiding this comment

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

But if we bump the version of each crate (and all its transitive dependants) in each MR, then we will have huge merge conflicts.
I think if we instead note the version bump in the prdoc then it can happen automatically at release process.

It would also require is to backport the version bumps from crates-io, leading to even more conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

It would also require is to backport the version bumps from crates-io, leading to even more conflicts.

Hmm? Why would we need to backport version bumps? I mean the version isn't bumping automatically.

I think if we instead note the version bump in the prdoc then it can happen automatically at release process.

If you change something deep inside the tree, I don't want to copy past 300 crate names and their bumps to a prdoc file...

But if we bump the version of each crate (and all its transitive dependants) in each MR, then we will have huge merge conflicts.

We should not bump in every pr. Bumps should only be done as they are required between releases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm? Why would we need to backport version bumps? I mean the version isn't bumping automatically.

Yea I see. I thought about using prdoc to not the semver bump of each crate that i changed, and a script can then take the maximum of all bumps and apply them.

If you change something deep inside the tree, I don't want to copy past 300 crate names and their bumps to a prdoc file...

It could suffice to note the changed crates - all dependant ones could bump automatically.

We should not bump in every pr. Bumps should only be done as they are required between releases.

Okay yea i read it in the other MR from you. So:

  • developer changes one crate and bumps that.
  • CI reminds him to not bump multiple times but to also bump all dependant crates
  • CI checks that there are no obvious and unintended breaking changes

Copy link
Member

Choose a reason for hiding this comment

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

It could suffice to note the changed crates - all dependant ones could bump automatically.

Bumping the dependent crates is not that easy. I mean you only need to bump the dependent crates if you re-export anything as public api.

Copy link
Member Author

Choose a reason for hiding this comment

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

But when we change something in sp-io for example, without modifying the public API but some implementation detail, then we should bump all dependants, or?
I am not sure if its easy to manually select which crates to bump, since there is the chance of missing some. If we always bump all dependants then maybe we bump too many, but at least we never miss any.

Copy link
Contributor

@liamaharon liamaharon Nov 27, 2023

Choose a reason for hiding this comment

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

We should not bump in every pr. Bumps should only be done as they are required between releases.

Assuming we are talking about the stable branch here, I disagree for two reasons

  1. It will be much easier for the developer who made each PR to confidently know which crates changed and how, and for them to make targeted bumps in their PR.

    Otherwise, release team will need to do a massive effort identifying and bumping crates based on changes of many PRs from a long time ago they don't understand. This I feel is doomed to fail and is unfair to the release team.

    Also, the developer will be expected to write a PRDoc right? If they have the knowledge to write a PRDoc, they may as well bump appropriate crates while they're at it. Otherwise it's just leaving unnecessary extra work for later.

  2. We should strive for automated releases when code is merged to master, no more manual release business. This requires the developer to bump their crates when they merge to to master.

But when we change something in sp-io for example, without modifying the public API but some implementation detail, then we should bump all dependants, or?

Yes, I think we should. There are tools to automate these common workspace release tasks: https://github.com/Byron/cargo-smart-release https://github.com/crate-ci/cargo-release https://github.com/pksunkara/cargo-workspaces

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it now to make the developer update the crates versions in their Merge Requests (where necessary).

Copy link
Contributor

@liamaharon liamaharon Dec 2, 2023

Choose a reason for hiding this comment

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

FYI working on a tool for this https://github.com/liamaharon/cargo-workspace-version-tools

Currently able to sync (catch up) our Cargo.toml versions with crates.io. Next will add functionality to bump a crate and automatically have dependent crates also bumped.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
docs/AUDIT.md Show resolved Hide resolved
@ggwpez
Copy link
Member Author

ggwpez commented Nov 25, 2023

One thing we should think about is to name things which are "unstable" internal api. Aka things that can be merged to master faster even when it requires a major version bump. I'm thinking about the Polkadot node internals that are not really exposed.

So it would only change the behaviour of a polkadot node, but not the behaviour of any of our public APIs?
Is the use-case for this just bug fixes to the node or also adding features?
Not sure if we need to include CLI flags, would probably be good if we consider them part of our external API.

docs/RELEASE.md Outdated Show resolved Hide resolved
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Would be nice to add a note about testnet updates. Do they update on nightly or mainline?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Nov 27, 2023

Would be nice to add a note about testnet updates. Do they update on nightly or mainline?

They need to be mentioned explicitly! Good point!

ggwpez and others added 4 commits November 27, 2023 13:01
Co-authored-by: joe petrowski <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
docs/AUDIT.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez marked this pull request as ready for review December 4, 2023 12:24
docs/RELEASE.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/AUDIT.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated
Comment on lines 39 to 50
## Crate Bumping

Cadence: (possibly) each Merge Request. Responsible: Developer that opened the MR.

Following SemVer isn't easy, but there exists [a guide](https://doc.rust-lang.org/cargo/reference/semver.html) in the Rust documentation that explains the small details on when to bump what. This process should be augmented with CI checks that utilize [`cargo-semver-checks`](https://github.com/obi1kenobi/cargo-semver-checks) and/or [`cargo-public-api`](https://github.com/Enselic/cargo-public-api). They must also pay attention to downstream dependencies that require a version bump, because they export the changed API.

### Steps

1. Developer opens a Merge Request with changed crates against `master`.
2. They bump all changed crates according to SemVer.
3. They bump all crates that export any changed types in their *public API*.
4. They also bump all crates that inherit logic changes from relying on one of the bumped crates.
Copy link
Contributor

@liamaharon liamaharon Dec 4, 2023

Choose a reason for hiding this comment

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

I'm confused by this.

Shouldn't crate bumping only be necessary when merging to the release branch, and master bumps are just automated nightly releases?

Do you mean to refer to the release branch instead of master branch here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to describe the normal workflow of merging a standard Merge request.

Currently we never touch the Cargo.toml version. But in the future we would need to increment it according to SemVer. This can be a major, minor or patch bump - depending on the change.

Otherwise when would we bump these versions? We dont really "merge" to the release branch, unless its a backport.

Copy link
Contributor

@liamaharon liamaharon Dec 4, 2023

Choose a reason for hiding this comment

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

My understanding was

  • merge to master: don't need to do bumps on merge. releases will be made nightly bumping the semver with a new -nightlyYYYY-MM-DD suffix. I don't see benefit of bumping X.Y.Z between nightly releases, because the code will have a breaking bump (from the -nightly change) soon anyway.
  • merge to release: we need to bump according to X.Y.Z semver, because we need to make new releases for the changed crates.

Copy link
Contributor

@liamaharon liamaharon Dec 4, 2023

Choose a reason for hiding this comment

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

This is how I understood it, where master branch only get nightly releases.

each crates versions like this:

stable
v1.0.0 --------> v1.0.1 ------------------------> v2.0.0 ---------------------- -> ...
                                                    ^
                                             CLOBBER|
master                                              |
v2.0.0-nightly2023-01-01 -> ... v2.0.0-nightly2023-04-01 -> v3.0.0-nightly2023-04-02 -> ...

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to bump according to X.Y.Z semver, because we need to make new releases for the changed crates.

But then how do we keep track of these in the meantime? Yes sure we dont need to bump for the nightly releases since they get their own version number overwritten.
We would also need to bullet proof automate these version bump then. I would also like to do this with prdoc and so on, but @bkchr seems to be in favour of just bumping them directly. Note that we dont need to bump multiple times on master if there was no release so far.

Copy link
Member

@gavofyork gavofyork Dec 12, 2023

Choose a reason for hiding this comment

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

So we would still bump only major every 3 months?

In my proposal, yes.

In my proposal:

  • Major bumps on release every 3 months when the Clobber happens. SemVer respected.
  • Minor bumps on release every 2 weeks for any crate touched by a backport or any crate depending on such a crate. Backport reviews make an explicit check that they do not make breaking changes. SemVer respected.
  • Patch bumps on release for out-of-band security fixes which have been reviewed as non-breaking. SemVer respected.
  • Nightly just has the next major version with a -nightly suffix. SemVer respected.

Copy link
Member Author

@ggwpez ggwpez Jan 2, 2024

Choose a reason for hiding this comment

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

My understanding from the forum post is this was the plan yes: master is never bumped manually, it's only bumped by the CI every nightly release.

The nightly is a pre-release, since it has a dash in the version name. It is completely irrelevant for any SemVer considerations and can break at any time.

For example, if a crate version in stable is 1.2.1, master would be set to 2.0.0-nightlyYYYY-MM-DD.

Yes, but only temporarily in the CI. This bump is never actually committed.

Then every 6th release, we remove the -nightlyYYYY-MM-DD from the versions and that becomes our new stable branch.

As you said below, this could introduce major bump without breaking changes. But maybe it would still be a good tradeoff if the other way introduces too much overhead. I mainly dont like it much because it breaks cargo update.

This way, we don't need to worry at all about breaking changes when we merge to master.

We dont have to "worry" about them, but we still have to mark them as breaking.

In my proposal:
Major bumps on release every 3 months when the Clobber happens. SemVer respected.

Just as note: I interpret this as meaning that unnecessary major bump should be avoided, since SemVer does not require them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would really first like to try out the tooling and then see what we should do. I mean the tooling probably already works for most of the things we are doing as breaking changes, like changing a function name or removing a parameter.

Okay then lets try the native bumping without Prdoc for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update this to clarify manual 'crate bumping' applies to backports into stable not master?

Copy link
Member 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? We would bump crates in the MR that merges into master.
Then that version bump could get back ported to stable, but its version should not need any tweaking.

docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
@pavelsupr
Copy link
Contributor

@bkchr @ggwpez,

Security wanted to propose one enhancement, targeted on the release pipelines from the integrity perspective.

Looks like, this proposition doesn't affect the subject of this thread too much, but it can cause some minor adjustments for this thread.

Could you please check this issue for the details, and comment on that issue directly? Since this PR is public. Here we can post the final outcomes only.

docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Generally looking good.

Still some questions around backporting. I think we should also backport other things, that do not break APIs. If in between there was a big change on master, we can probably stop backporting for most of the stuff until we do next clobber. We also should highlight on what is internal api, like certain crates from Polkadot node and thus they are fine to break/do major releases in between on the release branch.

And what about westend? This will follow the release branch?

README.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Show resolved Hide resolved
docs/RELEASE.md Outdated
Comment on lines 21 to 24
out of band release that fixes some critical bug. The node version is not following SemVer.
This means that the version doesn't express if there are any breaking changes in the CLI
interface or similar. The node version is declared in the `NODE_VERSION` variable in
`polkadot/node/primitives/src/lib.rs`.
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why doesn't the node follow semver?

What would be the point and what would be included in this? RPC? Cli? Some internal changes?

2. Why do we put the node version in NODE_VERSION instead of just in the Cargo.toml?

#1495 (comment)

docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
docs/RELEASE.md Outdated Show resolved Hide resolved
ggwpez and others added 2 commits January 23, 2024 13:45
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 3/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5006581

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
README.md Outdated Show resolved Hide resolved
docs/AUDIT.md Outdated Show resolved Hide resolved
ggwpez and others added 2 commits January 24, 2024 13:43
Co-authored-by: Jegor Sidorenko <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez added this pull request to the merge queue Jan 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 26, 2024
@ggwpez ggwpez added this pull request to the merge queue Jan 26, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 26, 2024
Writing down the processes to do our releases.

Status: please review & approve so we can go ahead.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Liam Aharon <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Jegor Sidorenko <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 26, 2024
@ggwpez ggwpez added this pull request to the merge queue Jan 26, 2024
Merged via the queue into master with commit d72fb58 Jan 26, 2024
123 of 124 checks passed
@ggwpez ggwpez deleted the oty-release-doc branch January 26, 2024 22:15
al3mart pushed a commit that referenced this pull request Jan 29, 2024
Writing down the processes to do our releases.

Status: please review & approve so we can go ahead.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Liam Aharon <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Jegor Sidorenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T11-documentation This PR/Issue is related to documentation.
Projects
Status: Draft
Development

Successfully merging this pull request may close these issues.