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

Fix issues with testnet dir, update docs #992

Merged
merged 16 commits into from
Apr 17, 2020
Merged

Fix issues with testnet dir, update docs #992

merged 16 commits into from
Apr 17, 2020

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Apr 9, 2020

Issue Addressed

NA

Proposed Changes

Fixes a bug introduced in #912 (I assume) that broke some of the bn testnet ... subcommands and also the --testnet-dir command when the hardcoded testnet is invalid (which is the case now).

Notably, this PR doesn't fix bn testnet ... it only fixes --testnet-dir and then updates the docs to remove usages of bn testnet ... in favor of --testnet-dir + lcli new-testnet.

Additional Info

A later PR can strip out all the old bn testnet ... subcommands.

@paulhauner paulhauner added work-in-progress PR is a work-in-progress ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 9, 2020
@paulhauner paulhauner marked this pull request as ready for review April 9, 2020 04:39
@AgeManning AgeManning changed the base branch from master to v0.2.0 April 9, 2020 14:30
@AgeManning AgeManning changed the base branch from v0.2.0 to master April 9, 2020 14:31
@paulhauner paulhauner added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Apr 13, 2020
AgeManning added a commit that referenced this pull request Apr 14, 2020
Co-Authored-By: Age Manning <Age@AgeManning.com>
@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 14, 2020
@paulhauner
Copy link
Member Author

Purging has been updated @michaelsproul :)

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Just a few small nits before I test it out (it looks like it should work)

@@ -221,7 +221,7 @@ where
.get::<PersistedBeaconChain>(&Hash256::from_slice(&BEACON_CHAIN_DB_KEY))
.map_err(|e| format!("DB error when reading persisted beacon chain: {:?}", e))?
.ok_or_else(|| {
"No persisted beacon chain found in store. Try purging the beacon chain database."
"No persisted beacon chain found in store. Try deleting the .lighthouse/beacon dir."
Copy link
Member

Choose a reason for hiding this comment

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

I think this should stay as it was

@@ -294,6 +291,39 @@ pub fn get_config<E: EthSpec>(
client_config.network.discovery_address =
Some("127.0.0.1".parse().expect("Valid IP address"))
}

/*
* Load the eth2 testnet dir to obtain some addition config values.
Copy link
Member

Choose a reason for hiding this comment

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

addition -> additional

@@ -1,70 +1 @@
# Simple Local Testnet
Copy link
Member

Choose a reason for hiding this comment

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

This file is just a heading, I think we should delete it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! mdbook has an annoying habit of creating a file if it's listed in SUMMARY.md but doesn't exist.

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 17, 2020
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Tested it and it works a treat! Happy to merge once my minor comments are addressed

@paulhauner paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 17, 2020
@paulhauner
Copy link
Member Author

All comments addressed! :)

@paulhauner paulhauner added ready-to-squerge and removed ready-for-review The code is ready for review labels Apr 17, 2020
@paulhauner paulhauner merged commit 1a3d1b3 into master Apr 17, 2020
@paulhauner paulhauner deleted the new-testnet-dir branch April 17, 2020 07:49
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.

None yet

3 participants