Removes already applied migrations #118

bkchr · 2023-12-11T10:10:16Z

No description provided.

acatangiu

~~Looks good, but let's merge after we see them applied on-chain.~~

Wait I'm confused, lemme double check this

acatangiu

I saw comment #68 (comment) where it says you want to merge this PR for pending 1.1.0 release, but some migrations here were never released (they were added after 1.0.0 release).

So we should not include/merge this PR (at least in its current form) before 1.1.0 release.

acatangiu · 2023-12-11T14:33:04Z

relay/polkadot/src/lib.rs

-	/// Upgrade Session keys to include BEEFY key.
-	/// When this is removed, should also remove `OldSessionKeys`.
-	pub struct UpgradeSessionKeys;
-	impl frame_support::traits::OnRuntimeUpgrade for UpgradeSessionKeys {
-		fn on_runtime_upgrade() -> Weight {
-			Session::upgrade_keys::<OldSessionKeys, _>(transform_session_keys);
-			Perbill::from_percent(50) * BlockWeights::get().max_block
-		}
-	}


This migration was never released (and obviously never enacted on-chain) - don't remove it before we see it enacted.

Haha, I didn't wanted to remove it :P I even left it in the list below :D Will readd the code, ty!

acatangiu · 2023-12-11T15:04:08Z

FYI: opened a PR for a doc with a quick guide that IMO we should follow to make our lives easier: #120

acatangiu · 2023-12-11T15:12:31Z

relay/polkadot/src/lib.rs

 		pallet_nomination_pools::migration::versioned_migrations::V5toV6<Runtime>,
 		pallet_nomination_pools::migration::versioned_migrations::V6ToV7<Runtime>,
-
-		runtime_parachains::scheduler::migration::v1::MigrateToV1<Runtime>


not sure how to check if this was already run on-chain, but I see it was added here after the last release:

active on-chain is spec polkadot/1_000_001 which was released on 22.10.2023

added in Fix migrations and add migration CI checks #75 merged a month later on 22.11.2023

This was added after the release, to switch back to the fixed upstream one. However, I had included the fixed upstream migration directly in this repo here: https://github.com/polkadot-fellows/runtimes/pull/26/files#diff-efa4caeb17487ecb13d8f5eb7863c3241d84afa2e73fbf25909a2ca89df0f362R1739

acatangiu · 2023-12-11T15:14:54Z

system-parachains/asset-hubs/asset-hub-kusama/src/lib.rs

-pub type Migrations = (
-	pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
-	pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
-	InitStorageVersions,


maybe also remove struct InitStorageVersions defined and implemented below

(same for other system paras)

liamaharon · 2023-12-12T10:02:02Z

system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs

@@ -817,49 +817,14 @@ pub type UncheckedExtrinsic =
 	generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
 /// Migrations to apply on runtime upgrade.
 pub type Migrations =
-	(pallet_collator_selection::migration::v1::MigrateToV1<Runtime>, InitStorageVersions);
+	frame_support::migrations::VersionedMigration<0, 1, UniquesMigration, Uniques, RocksDbWeight>;


If the InitStorageVersions was executed, we need to reset the Uniques on-chain version back to 0 before running this.

No this init was never executed on chain. This was the reason the CI was failing for this pr.

I wonder why it was not, I can see it here - https://github.com/polkadot-fellows/runtimes/blob/v1.0.0/system-parachains/asset-hubs/asset-hub-polkadot/src/lib.rs#L799

Yes the migration was there. But the init of Uniques was added here: https://github.com/polkadot-fellows/runtimes/pull/87/files#diff-2dc481414ffabfb3b0b2c24f3c0cfc8a8b3ed6b56689a696d02be635795f5cb6R857

ggwpez

I dont know why we use --disable-idempotency-checks in the CI migration checks.
I will try it in a MR to see if we can enable it now (maybe at least for some runtimes).

bkchr · 2024-01-04T12:51:51Z

I dont know why we use --disable-idempotency-checks in the CI migration checks.

https://github.com/polkadot-fellows/runtimes/pull/75/files#diff-9c4bfd050ca12885b582fbfd04536460a7a4a0770824ee46694010b1e4e53287R74

ggwpez · 2024-01-04T13:15:59Z

I dont know why we use --disable-idempotency-checks in the CI migration checks.

https://github.com/polkadot-fellows/runtimes/pull/75/files#diff-9c4bfd050ca12885b582fbfd04536460a7a4a0770824ee46694010b1e4e53287R74

Yea I cannot read 🙈 removed in #133

ggwpez · 2024-01-04T14:15:16Z

@muharem its not counting your approval since your on-chain identity does not contain a github in the additional field.

sandreim · 2024-01-05T09:30:02Z

@muharem its not counting your approval since your on-chain identity does not contain a github in the additional field.

it's not counting mine as well and I have my github in the additional field

ordian

Checked polkadot migration list in 1.0.1 and it checks out.

ordian · 2024-01-05T09:37:49Z

relay/polkadot/src/lib.rs

-		parachains_configuration::migration::v9::MigrateToV9<Runtime>,
-		// Migrate parachain info format
-		paras_registrar::migration::VersionCheckedMigrateToV1<Runtime, ParachainsToUnlock>,
-
 		// Upgrade SessionKeys to include BEEFY key
 		UpgradeSessionKeys,


is there an issue to track the removal of this in the next release so we don't forget?

No we don't have an issue for this, but in the fellowship repo we can just remove always all the migrations.

bkchr · 2024-01-05T10:08:42Z

@muharem its not counting your approval since your on-chain identity does not contain a github in the additional field.

it's not counting mine as well and I have my github in the additional field

The logs say something different:

ggwpez,acatangiu,kianenigma,shawntabrizi,gavofyork,davxy,eskimor,edwardmack,andresilva,pepyakin,rphmeier,bkchr,sandreim,tomaka,agryaznov,xlc,arkpar,ordian,KiChjang,noot,joepetrowski
Latest reviews are [{"user":"acatangiu","state":"APPROVED"},{"user":"joepetrowski","state":"APPROVED"},{"user":"muharem","state":"APPROVED"},{"user":"ggwpez","state":"APPROVED"},{"user":"sandreim","state":"APPROVED"},{"user":"ordian","state":"APPROVED"}]

Enable one more check in the CI to ensure that our migrations are idempotent. Can only merge after #118 --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]>

bkchr added 2 commits December 11, 2023 11:09

Removes already applied migrations

4056dc1

FMT

b900d75

bkchr mentioned this pull request Dec 11, 2023

1.1.0 Release #68

Closed

acatangiu approved these changes Dec 11, 2023

View reviewed changes

acatangiu suggested changes Dec 11, 2023

View reviewed changes

Readd accidentially removed code

dd23159

bkchr requested a review from acatangiu December 11, 2023 15:11

No warnings

71a0af4

acatangiu reviewed Dec 11, 2023

View reviewed changes

Remove unused migrations

b4e8bd9

acatangiu approved these changes Dec 11, 2023

View reviewed changes

bkchr added 3 commits December 11, 2023 17:46

FMT

a4d1504

Adds uniques migration

3334957

🤦

fb34ebc

liamaharon reviewed Dec 12, 2023

View reviewed changes

joepetrowski approved these changes Dec 12, 2023

View reviewed changes

Merge branch 'main' into bkchr-cleanup-migrations

39e40f5

muharem approved these changes Dec 13, 2023

View reviewed changes

bkchr requested a review from ggwpez January 4, 2024 12:05

ggwpez approved these changes Jan 4, 2024

View reviewed changes

ggwpez mentioned this pull request Jan 4, 2024

CI: Enable try-runtime idempotency check #133

Merged

Merge branch 'main' into bkchr-cleanup-migrations

3005a97

bkchr enabled auto-merge (squash) January 4, 2024 13:29

Merge branch 'main' into bkchr-cleanup-migrations

89d3471

sandreim approved these changes Jan 5, 2024

View reviewed changes

Merge branch 'main' into bkchr-cleanup-migrations

3bf86dc

ordian approved these changes Jan 5, 2024

View reviewed changes

Merge branch 'main' into bkchr-cleanup-migrations

96dc830

bkchr merged commit 3caec69 into main Jan 5, 2024
14 checks passed

ggwpez deleted the bkchr-cleanup-migrations branch January 5, 2024 11:31

ggwpez added a commit that referenced this pull request Jan 9, 2024

CI: Enable try-runtime idempotency check (#133)

fa59883

Enable one more check in the CI to ensure that our migrations are idempotent. Can only merge after #118 --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes already applied migrations #118

Removes already applied migrations #118

bkchr commented Dec 11, 2023

acatangiu left a comment •

edited

Loading

acatangiu left a comment

acatangiu Dec 11, 2023

bkchr Dec 11, 2023

acatangiu commented Dec 11, 2023

acatangiu Dec 11, 2023

bkchr Dec 11, 2023

acatangiu Dec 11, 2023

bkchr Dec 11, 2023

liamaharon Dec 12, 2023

bkchr Dec 12, 2023

muharem Dec 13, 2023

bkchr Dec 13, 2023

ggwpez left a comment •

edited

Loading

bkchr commented Jan 4, 2024

ggwpez commented Jan 4, 2024

ggwpez commented Jan 4, 2024

sandreim commented Jan 5, 2024

ordian left a comment

ordian Jan 5, 2024

bkchr Jan 5, 2024

bkchr commented Jan 5, 2024

Removes already applied migrations #118

Removes already applied migrations #118

Conversation

bkchr commented Dec 11, 2023

acatangiu left a comment • edited Loading

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez left a comment • edited Loading

Choose a reason for hiding this comment

bkchr commented Jan 4, 2024

ggwpez commented Jan 4, 2024

ggwpez commented Jan 4, 2024

sandreim commented Jan 5, 2024

ordian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented Jan 5, 2024

acatangiu left a comment •

edited

Loading

ggwpez left a comment •

edited

Loading