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.

Sign up for GitHub

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

Feat: Lotus Daemon CLI: Auto remove existing chain if importing chain file or snapshot #11277

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Feat: Lotus Daemon CLI: Auto remove existing chain if importing chain file or snapshot #11277

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0033544
Auto remove existing chain if importing chain file or snapshot
mb1896 Sep 19, 2023
10a9eb2
Address review comments
mb1896 Sep 20, 2023
bdffac3
Add to changelog
mb1896 Sep 20, 2023
5b99147
Fix lint
mb1896 Sep 21, 2023
49044b4
Address review comments, again
mb1896 Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ This feature release requires a **minimum Go version of v1.19.12 or higher to su
- feat: Lotus Gateway: add MpoolPending, ChainGetBlock and MinerGetBaseInfo ([filecoin-project/lotus#10929](https://github.com/filecoin-project/lotus/pull/10929))

## Improvements
- chore: Auto remove local chain data when importing chain file or snapshot ([filecoin-project/lotus#11277](https://github.com/filecoin-project/lotus/pull/11277))
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for including this! It needs to be moved up to the UNRELEASED section, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed

- chore: update ffi & fvm ([filecoin-project/lotus#11040](https://github.com/filecoin-project/lotus/pull/11040))
- feat: Make sure we don't store duplidate actor events caused to reorgs in events.db ([filecoin-project/lotus#11015](https://github.com/filecoin-project/lotus/pull/11015))
- sealing: Use only non-assigned deals when selecting snap sectors ([filecoin-project/lotus#11002](https://github.com/filecoin-project/lotus/pull/11002))
Expand Down
39 changes: 32 additions & 7 deletions cmd/lotus/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,37 @@ var DaemonCmd = &cli.Command{
}
}

if cctx.Bool("remove-existing-chain") {
chainfile := cctx.String("import-chain")
snapshot := cctx.String("import-snapshot")
willImportChain := false
if chainfile != "" || snapshot != "" {
if chainfile != "" && snapshot != "" {
return fmt.Errorf("cannot specify both 'import-snapshot' and 'import-chain'")
}
willImportChain = true
}

willRemoveChain := cctx.Bool("remove-existing-chain")
if willImportChain && !willRemoveChain {
// Confirm with the user about the intention to remove chain data.
reader := bufio.NewReader(os.Stdin)
fmt.Print("Importing chain or snapshot will by default delete existing local chain data. Do you want to proceed and delete? (yes/no): ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Print("Importing chain or snapshot will by default delete existing local chain data. Do you want to proceed and delete? (yes/no): ")
fmt.Print("Do you want to delete existing local chain data as part of the import process? (yes/no): ")

Copy link
Contributor

Choose a reason for hiding this comment

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

(just a bit more clear, there isn't really a default since we're forcing users to specify)

Copy link
Contributor Author

@mb1896 mb1896 Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You are right, however we couldn't differentiate between

  • user not specifying a flag's value
  • user specifically specifying a flag to false

That's why we have to ask for the user's input...

userInput, err := reader.ReadString('\n')
if err != nil {
return xerrors.Errorf("reading user input: %w", err)
}
userInput = strings.ToLower(strings.TrimSpace(userInput))

if userInput == "yes" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to handle case-sensitivity here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ToLower() function handled it.

willRemoveChain = true
} else if userInput == "no" {
willRemoveChain = false
} else {
return fmt.Errorf("invalid input. please answer with 'yes' or 'no'")
}
}

if willRemoveChain {
lr, err := repo.NewFS(cctx.String("repo"))
if err != nil {
return xerrors.Errorf("error opening fs repo: %w", err)
Expand All @@ -289,12 +319,7 @@ var DaemonCmd = &cli.Command{
}
}

chainfile := cctx.String("import-chain")
snapshot := cctx.String("import-snapshot")
if chainfile != "" || snapshot != "" {
if chainfile != "" && snapshot != "" {
return fmt.Errorf("cannot specify both 'import-snapshot' and 'import-chain'")
}
if willImportChain {
var issnapshot bool
if chainfile == "" {
chainfile = snapshot
Expand Down