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

ci: check runtime migrations#14618

Merged
liamaharon merged 10 commits intomasterfrom
liam-ci-check-runtime-upgrade
Jul 27, 2023
Merged

ci: check runtime migrations#14618
liamaharon merged 10 commits intomasterfrom
liam-ci-check-runtime-upgrade

Conversation

@liamaharon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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)

@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
Copy Markdown
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)

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
Copy Markdown
Contributor Author

bot merge

@paritytech-processbot
Copy link
Copy Markdown

Error: Statuses failed for bc1b0f7

@liamaharon
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

bot merge

@paritytech-processbot
Copy link
Copy Markdown

Error: Statuses failed for 1448876

@liamaharon
Copy link
Copy Markdown
Contributor Author

bot merge --force

@command-bot
Copy link
Copy Markdown

command-bot bot commented Jul 27, 2023

@liamaharon error: unknown option '--force'

@liamaharon liamaharon merged commit 46136f2 into master Jul 27, 2023
@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