Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

ci: check runtime migrations #14618

Merged
merged 10 commits into from
Jul 27, 2023
Merged

ci: check runtime migrations #14618

merged 10 commits into from
Jul 27, 2023

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Jul 24, 2023

Partial paritytech/polkadot-sdk#481

Creates CI jobs to help ensure a substrate change does not accidentally break runtime upgrades on polkadot/kusama/westend/rococo.

e.g. if a pallet is updated to a new version and requires a migration but no migration is included in a companion PR to apply the migration on-chain, the CI will fail.

@liamaharon liamaharon requested a review from a team as a code owner July 24, 2023 07:18
@liamaharon liamaharon added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 24, 2023
@liamaharon liamaharon changed the title [DNM] ci check runtime upgrade [DNM] ci: check runtime upgrades Jul 24, 2023
@liamaharon liamaharon changed the title [DNM] ci: check runtime upgrades [DNM] ci: check runtime migrations Jul 24, 2023
@liamaharon liamaharon added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 24, 2023
@liamaharon liamaharon changed the title [DNM] ci: check runtime migrations ci: check runtime migrations Jul 24, 2023
@ggwpez
Copy link
Member

ggwpez commented Jul 24, 2023

This does not quite prevent it, since we can still forget to bump the pallet version, or?
But we can add a subsequent job to check that if the storage metadata of a pallet was changed that the version got bumped.

e.g. if a pallet is updated to a new version and requires a migration but no migration is included in a companion PR to apply the migration on-chain, the CI will fail.

We already have this check in Polkadot, now you also want to run it in the companion CI or what exactly is this changed aiming at?

@liamaharon
Copy link
Contributor Author

liamaharon commented Jul 24, 2023

The check we added in polkadot will catch if we accidentally break it in a polkadot PR, but it will not catch if we accidentally push a change in substrate (e.g. update the pallet version) but forget to create a valid companion PR.

This recently happened in this substrate PR #14251, where the migration was written but it was forgotten to add the migration to the Migrations tuple in the companion: paritytech/polkadot#7309 (comment)

scripts/ci/gitlab/pipeline/build.yml Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team July 25, 2023 14:14
@liamaharon liamaharon requested a review from alvicsam July 25, 2023 16:02
@paritytech-ci paritytech-ci requested review from a team July 26, 2023 10:58
Copy link
Contributor

@alvicsam alvicsam left a comment

Choose a reason for hiding this comment

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

A small cleanup (this rule is defined in the .test-refs-no-trigger-prs-only)

scripts/ci/gitlab/pipeline/build.yml Outdated Show resolved Hide resolved
scripts/ci/gitlab/pipeline/build.yml Outdated Show resolved Hide resolved
scripts/ci/gitlab/pipeline/build.yml Outdated Show resolved Hide resolved
scripts/ci/gitlab/pipeline/build.yml Outdated Show resolved Hide resolved
liamaharon and others added 4 commits July 26, 2023 21:11
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
Co-authored-by: Alexander Samusev <41779041+alvicsam@users.noreply.github.com>
@liamaharon liamaharon requested a review from alvicsam July 26, 2023 11:27
@liamaharon
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for bc1b0f7

@liamaharon
Copy link
Contributor Author

I'm going to wait for the Rococo job to fix before merging this so it doesn't cause all commits to have a ❌.

The Rococo job will fix after .43 is deployed there, which @al3mart is planning this week.

@liamaharon
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for 1448876

@liamaharon
Copy link
Contributor Author

bot merge --force

@command-bot
Copy link

command-bot bot commented Jul 27, 2023

@liamaharon error: unknown option '--force'

@liamaharon liamaharon merged commit 46136f2 into master Jul 27, 2023
3 checks passed
@liamaharon liamaharon deleted the liam-ci-check-runtime-upgrade branch July 27, 2023 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants