Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

feat: use try-runtime-cli instead of command #270

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Conversation

Moliholy
Copy link
Contributor

Closes #263.

@Moliholy Moliholy added the migration This pull request has a storage migration label Aug 29, 2023
@Moliholy Moliholy requested a review from hbulgarini August 29, 2023 16:21
@Moliholy Moliholy self-assigned this Aug 29, 2023
@Moliholy
Copy link
Contributor Author

Added the migration label to check if it works.

@Moliholy Moliholy requested a review from stiiifff August 29, 2023 16:22
@hbulgarini
Copy link
Contributor

nice @Moliholy ! thanks for this!

quick question: Are the try-runtime sub commands defined in the node still necessary or we can clean them up?

https://github.com/paritytech/trappist/blob/main/node/src/command.rs#L390:L472

@hbulgarini
Copy link
Contributor

CC: @brunopgalvao

@Moliholy
Copy link
Contributor Author

quick question: Are the try-runtime sub commands defined in the node still necessary or we can clean them up?

@hbulgarini to be honest I wasn't totally sure we should remove the command just yet. Nothing in particular, I simply thought to take a conservative approach and leave both ways to run the try-runtime, just in case.

Anyhow, let's see what @brunopgalvao says.

@brunopgalvao
Copy link
Contributor

brunopgalvao commented Sep 1, 2023

Thanks for your work here @Moliholy

try-runtime is working nicely with these changes.

Three options that I see:

  1. Deprecation of the sub command similar to here:
  1. Remove the sub command since it is working now with the standalone CLI
  2. Remove the sub command in a later release

I am more in favor of 2.

@Moliholy Moliholy force-pushed the feat/try-runtime-cli branch from b713299 to 209a91c Compare September 5, 2023 09:37
@Moliholy
Copy link
Contributor Author

Moliholy commented Sep 5, 2023

Three options that I see:

  1. Deprecation of the sub command similar to here:
  1. Remove the sub command since it is working now with the standalone CLI
  2. Remove the sub command in a later release

I am more in favor of 2.

Me too. I just added c85650a with the changes.

@Moliholy Moliholy force-pushed the feat/try-runtime-cli branch from 209a91c to c85650a Compare September 5, 2023 09:41
@Moliholy
Copy link
Contributor Author

Moliholy commented Sep 5, 2023

I also added 2d53a48 to remove already applied migrations. Otherwise the migration test won't pass.

@stiiifff stiiifff merged commit 9e2414e into main Sep 7, 2023
@stiiifff stiiifff deleted the feat/try-runtime-cli branch September 7, 2023 08:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
migration This pull request has a storage migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the standalone try-runtime CLI
4 participants