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

A0-1846: Removed migrations from release-11 branch #1324

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

Marcin-Radecki
Copy link
Contributor

Description

Remove migrations that were introduced in r-11.3 release.

@Marcin-Radecki Marcin-Radecki changed the title Removed migrations from release-11 branch A0-1846: Removed migrations from release-11 branch Aug 2, 2023
@Marcin-Radecki Marcin-Radecki requested review from kostekIV, DamianStraszak and h4nsu and removed request for DamianStraszak August 2, 2023 11:27
Copy link
Contributor

@kostekIV kostekIV left a comment

Choose a reason for hiding this comment

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

LGTM, there are changes in the scripts/synthetic-network that are not related to the migrations tho.

#[cfg(feature = "try-runtime")]
use {frame_support::ensure, pallets_support::ensure_storage_version, sp_std::vec::Vec};

use crate::{CommitteeSize, Config, NextEraCommitteeSize, Pallet, LOG_TARGET};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should remove migrations from our code. We should just detach them from the runtime...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That also I thought, yet I don't see now in aleph-node repo any other migrations, hance I thought to follow that rule and remove them. Let me check what substrate does with that

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the reason is we had this pallet hook based migrations before, which were removed to clean things up. But it seems the convention is to keep migrations, and just attach them when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have removed them quite some time ago. We have them in the git history if they will be ever needed (?). I have removed them because maintaining the old migrations is kinda meaningless and makes it harder to develop pallets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, in Substrate they indeed keep migrations. I'll will bring them back then

@Marcin-Radecki
Copy link
Contributor Author

LGTM, there are changes in the scripts/synthetic-network that are not related to the migrations tho.

True, will revert

@Marcin-Radecki Marcin-Radecki merged commit dc7118d into release-11 Aug 2, 2023
47 checks passed
@Marcin-Radecki Marcin-Radecki deleted the remove-migrations branch August 2, 2023 12:37
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.

3 participants