I think we should start binding migrations to crate versions, which should be relative easy after: paritytech/substrate#7208

This means, instead of having one extra storage item per migration, we just use the crate version to check which migrations should be executed.

However, this would require that we always bump some part of the crate version for any major change, but I think this should be doable.

So it this would be guideline:
when breaking storage:

• write migration as:

// Here before there should already be a check that if storag_version is before latest supported then nothing can be done.
if get_storage_version() <= current_pallet_version {
    do_migration
}

• and then bump pallet version: if current version is release then bump new major, or if current_version is not release, bump patch (this is needed only because some crate like polkadot use master and not released version)

That means pallet will be released with some version: 3.0.4 for instance if 4 migration were written during the developement of the version 3 of the pallet.

Is that an OK guideline ?
cc @apopiak

In general I would say that you should bump the minor version for stuff that requires migrations. Maybe some really small stuff can only bump the patch version, but that should normally only be used for bug fixes.
We should probably also stick to normal semver for when and how to bump the version.

My intuition is also to bump (at least) the minor version for new migrations/storage changes and only use patches for fixes.

ok, if polkadot still follows master that means we have to bump minor version on every PR which does migration/storage changes, even when the minor version is not published. I'm ok with this.

related paritytech/substrate#7208

I think this PR can be closed due to paritytech/substrate#7208

The strategy now should be:

• bump crate version
• add the migration code under if condition:

fn on_runtime_upgrade() {
	const LATEST_SUPPORTED = StorageVersion::new(x, y, z);
	let storage_version = match Self::storage_version() {
		Some(v) if v >= LATEST_SUPPORTED => v,
		_ => // Log some error and return
	};

	if storage_version < PalletVersion::new(a, b, c) {
		// Do some migration.
	}

	...

	if storage_version < PalletVersion::new($new_crate_version) {
		// New migration to write
	}
}

(the code should be adapted if we support migrating from no storage version, but this should be temporary as now pallet automatically write their version in storage).

I wonder whether we want to offer even more convenient APIs based on the versions introduced by paritytech/substrate#7208?
I could see something like the following:

// migration is only executed if the pallet storage version is 2.0.0
#[migration(pallet=System, from=(2,0,0))]
fn migrate_to_u32_refcount() -> frame_support::weights::Weight {
    Account::<T>::translate::<(T::Index, u8, T::AccountData), _>(|_key, (nonce, rc, data)|
        Some(AccountInfo { nonce, refcount: rc as RefCount, data })
    );
    T::MaximumBlockWeight::get()
}

I'm ok to introduce new syntax but what you propose doesn't seems to work:
if you have one migration introduced at version 2.0.0, another migration introduced at version 4.0.0 which depend on the former, and the pallet on chain with storage version 1.0.0 you probably want to execute migration for 2.0.0 and then 4.0.0.

but how could the second migration be written: from 3.0.0 or from 2.0.0, and should the first migration write to storage the version 2.0.0 so that the second migration can be triggered.

Maybe we can write something like:
#[migration(from="1.0.0", to="2.0.0"]which would be executed if version in storage is between [1.0.0, 2.0.0[ and would set 2.0.0 to storage.
#[migration(from="2,0,0", to="4.0.0"] which would be executed if version in storage is between [2.0.0, 4.0.0[ and would set 4.0.0 to storage.
But at the same time the macro would just save a if condition and a set storage.