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

CI: Enable try-runtime idempotency check #133

Merged
merged 11 commits into from
Jan 9, 2024
Merged

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 4, 2024

Enable one more check in the CI to ensure that our migrations are idempotent. Can only merge after #118

ggwpez added 2 commits January 4, 2024 13:44
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez marked this pull request as ready for review January 4, 2024 13:09
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez
Copy link
Member Author

ggwpez commented Jan 4, 2024

Looks like the spec-version check fails. Should i just bump all the versions by one in this MR or disable the check again @bkchr ?

@bkchr
Copy link
Contributor

bkchr commented Jan 4, 2024

IMO it would cool if we could enable this check optionally based on if the pr wants to release a new version. See here on how to detect if the changelog changes from unreleased to "released".

ggwpez added 2 commits January 4, 2024 18:46
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@s0me0ne-unkn0wn
Copy link
Contributor

Not sure if it's the right place to raise this question, but it's close enough. Currently, #56 bumps frame-system to 24.0.0 in this release. But to ensure the idempotency of migrations requiring spec version checks (see paritytech/polkadot-sdk#2265) we need frame-system to be at least 27.0.0. I'm not familiar with this versioning, so I'm afraid to screw things up if I do that bump myself without any guidance (especially considering I've already broken everything locally when I tried). So what's the procedure for bumping FRAME and Substrate crate versions in runtimes?

@ordian do we need to push with im-online removal in this very release?

@ggwpez
Copy link
Member Author

ggwpez commented Jan 5, 2024

But to ensure the idempotency of migrations requiring spec version checks (see paritytech/polkadot-sdk#2265) we need frame-system to be at least 27.0.0

I guess we have to delete it manually this time?

@s0me0ne-unkn0wn
Copy link
Contributor

I guess we have to delete it manually this time?

Not sure I follow, what do we delete manually?

@ggwpez
Copy link
Member Author

ggwpez commented Jan 5, 2024

I guess we have to delete it manually this time?

Not sure I follow, what do we delete manually?

The migration i mean. We delete it after it executed on-chain.

@s0me0ne-unkn0wn
Copy link
Contributor

The migration i mean. We delete it after it executed on-chain.

Ah yes. We can do that, sure. But that's not the only problem. Your PR enables mandatory idempotency checks. And without spec version checking API, my migration cannot be idempotent because the structure updated is not versioned. And that spec version checking API (last_runtime_upgrade_spec_version()) has been introduced in frame-system 27.0.0.

@bkchr
Copy link
Contributor

bkchr commented Jan 5, 2024

@ordian do we need to push with im-online removal in this very release?

No. I already told you this in element. Just wait for the next release.

@s0me0ne-unkn0wn
Copy link
Contributor

No. I already told you this in element. Just wait for the next release.

That's the elaboration of your answer I waited for! 😃 Thanks, we're just postponing it then.

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

ggwpez commented Jan 5, 2024

Okay so we basically have to ignore the idempotency check until the next runtime upgrade on Polkadot... I will selectively disable it.

@ggwpez ggwpez enabled auto-merge (squash) January 5, 2024 13:36
@ggwpez ggwpez merged commit fa59883 into main Jan 9, 2024
13 checks passed
@ggwpez ggwpez deleted the oty-try-runtime-checks branch January 9, 2024 15:47
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.

4 participants