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

Jump to bottom

feat(cmd): Provide ability to specify network to user #1073

Merged
merged 3 commits into from
Sep 13, 2022
Merged

feat(cmd): Provide ability to specify network to user #1073

merged 3 commits into from
Sep 13, 2022

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Sep 12, 2022 •
edited
Loading

Closes #1066 .

  • node store, extend home directory with network name
  • Keep CELESTIA_CUSTOM, deprecate CELESTIA_PRIVATE_GENESIS
    • panic will be taken care of by the node store naming extension
  • Add --node.network < arabica || mamaki || ... >
  • Fixes cel-shed and cel-key packages

@distractedm1nd distractedm1nd self-assigned this Sep 12, 2022
@distractedm1nd distractedm1nd added area:config CLI and config enhancement New feature or request kind:improvement and removed enhancement New feature or request labels Sep 12, 2022
renaynay
renaynay reviewed Sep 12, 2022
View reviewed changes
cmd/cel-key/node_types.go Show resolved Hide resolved
cmd/flags_node.go Outdated Show resolved Hide resolved
params/default.go Outdated Show resolved Hide resolved
@distractedm1nd distractedm1nd marked this pull request as ready for review September 12, 2022 13:53
@renaynay renaynay added kind:break! Attached to breaking PRs and removed kind:improvement labels Sep 12, 2022
renaynay
renaynay previously approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Tested, with and without CELESTIA_CUSTOM opts too

CELESTIA_CUSTOM will only panic if it sees a store of the same network name that has been initialised already :)

Wondertan
Wondertan requested changes Sep 12, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Requesting changes to keep the global state of params pkg being read-only and keeping network selection a Node instance specific. We should also keep defaults on the flags, so they explicit on the flag helps

cmd/flags_node.go Show resolved Hide resolved
cmd/flags_node.go Outdated Show resolved Hide resolved
cmd/flags_node.go Show resolved Hide resolved
params/default.go Show resolved Hide resolved
@distractedm1nd distractedm1nd mentioned this pull request Sep 12, 2022
Closed
Wondertan
Wondertan previously approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Thanks, and looking forward to implementation of #1076

cmd/flags_node.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1073 (a821b80) into main (8657edc) will increase coverage by 0.07%.
The diff coverage is 62.50%.

❗ Current head a821b80 differs from pull request most recent head 21c0be3. Consider uploading reports for the commit 21c0be3 to get more accurate results

@@            Coverage Diff             @@
##             main    #1073      +/-   ##
==========================================
+ Coverage   56.56%   56.63%   +0.07%     
==========================================
  Files         135      135              
  Lines        8965     8992      +27     
==========================================
+ Hits         5071     5093      +22     
- Misses       3359     3364       +5     
  Partials      535      535
Impacted Files Coverage Δ
params/default.go 13.88% <0.00%> (-0.82%) ⬇️
params/genesis.go 33.33% <ø> (ø)
cmd/flags_node.go 66.66% <59.09%> (-10.26%) ⬇️
cmd/celestia/bridge.go 70.58% <100.00%> (ø)
cmd/celestia/full.go 40.98% <100.00%> (ø)
cmd/celestia/light.go 40.98% <100.00%> (ø)
params/network.go 72.72% <100.00%> (+32.72%) ⬆️
ipld/get_namespaced_shares.go 88.32% <0.00%> (-2.19%) ⬇️
fraud/bad_encoding.go 66.00% <0.00%> (+3.00%) ⬆️
ipld/get_shares.go 100.00% <0.00%> (+10.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Wondertan
Wondertan approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Thanks!

params/network.go Show resolved Hide resolved
renaynay
renaynay approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

APPROOOOOVE

@distractedm1nd distractedm1nd merged commit faf5e51 into celestiaorg:main Sep 13, 2022
@distractedm1nd distractedm1nd mentioned this pull request Sep 13, 2022
Merged
distractedm1nd added a commit to renaynay/celestia-node that referenced this pull request Sep 19, 2022
@distractedm1nd
feat(cmd): Provide ability to specify network to user (celestiaorg#1073)
882c5c1 
* feat(cmd): adding network flag, deprecating CELESTIA_PRIVATE_GENESIS, changing default store extension

* fix(cmd): validating network passed with --node.network

* feat(cmd): dynamic network list for command hint
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Sep 21, 2022
@distractedm1nd
feat(cmd): Provide ability to specify network to user (celestiaorg#1073)
19e1da3 
* feat(cmd): adding network flag, deprecating CELESTIA_PRIVATE_GENESIS, changing default store extension

* fix(cmd): validating network passed with --node.network

* feat(cmd): dynamic network list for command hint
@renaynay renaynay mentioned this pull request Oct 24, 2022
Merged
renaynay added a commit that referenced this pull request Oct 24, 2022
@renaynay
refactor(nodebuilder): Allow custom networks to work over already-ini…
f4e582e 
…tialised store (#1270)

After conversation with @jbowen93 , we decided to allow using custom
networks over an already-initialised node store.

The issue this was originally attempting to prevent is accidental
corruption of chain data in the case that a user accidentally leaves
their environment variable set up and connects to another network over
an old node store.

This issue is actually currently prevented thanks to #1073 extending the
node store name with the network name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan approved these changes

@renaynay renaynay renaynay approved these changes

@liamsi liamsi Awaiting requested review from liamsi

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs is a code owner

Assignees

@distractedm1nd distractedm1nd

Labels
area:config CLI and config kind:break! Attached to breaking PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

params | cmd: Provide ability to specify network to user
4 participants
@distractedm1nd @codecov-commenter @Wondertan @renaynay