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

feature(cmd/gossamer): add aliases for cli flags #3098

Merged
merged 25 commits into from
Mar 14, 2023

Conversation

edwardmack
Copy link
Member

Changes

This PR contains changes to make gossamer more congruent with polkadot CLI commands in order to enable gossamer to work with paritytech zombienet testing framework.

  • Add CLI flag no-mdns to alias nomdns.
  • Add CLI flag base-path to alias basepath
  • Add CLI flag rpc-port to alias rpcport
  • Add CLI flag ws-port to alias wsport
  • Implement new CLI flag node-key which overrides the secret Ed25519 key to use for libp2p
  • Add unit tests for new flags

Tests

go test github.com/ChainSafe/gossamer/cmd/gossamer -run ^TestGlobalConfigFromFlags$ -v
go test github.com/ChainSafe/gossamer/cmd/gossamer -run ^TestNetworkConfigFromFlags$ -v
 go test github.com/ChainSafe/gossamer/cmd/gossamer -run ^TestRPCConfigFromFlags$ -v

Issues

Work done researching issue #2843

Primary Reviewer

@timwu20

@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Merging #3098 (c21a69d) into development (abe3601) will decrease coverage by 0.13%.
The diff coverage is 44.59%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #3098      +/-   ##
===============================================
- Coverage        51.62%   51.49%   -0.13%     
===============================================
  Files              219      219              
  Lines            28208    28278      +70     
===============================================
  Hits             14561    14561              
- Misses           12336    12394      +58     
- Partials          1311     1323      +12     

cmd/gossamer/config.go Outdated Show resolved Hide resolved
cmd/gossamer/config.go Outdated Show resolved Hide resolved
cmd/gossamer/config_test.go Show resolved Hide resolved
cmd/gossamer/config_test.go Show resolved Hide resolved
cmd/gossamer/config_test.go Show resolved Hide resolved
cmd/gossamer/flags.go Outdated Show resolved Hide resolved
dot/network/config.go Outdated Show resolved Hide resolved
dot/network/config.go Show resolved Hide resolved
cmd/gossamer/config.go Outdated Show resolved Hide resolved
cmd/gossamer/config.go Outdated Show resolved Hide resolved
@edwardmack edwardmack force-pushed the ed/cli_flags_update branch 2 times, most recently from bbf28da to 8a074d5 Compare February 9, 2023 20:20
@edwardmack edwardmack requested a review from qdm12 February 9, 2023 21:00
@edwardmack edwardmack force-pushed the ed/cli_flags_update branch 3 times, most recently from f846a17 to 39f0479 Compare February 10, 2023 16:06
dot/network/host.go Outdated Show resolved Hide resolved
dot/network/config.go Outdated Show resolved Hide resolved
dot/network/config.go Show resolved Hide resolved
cmd/gossamer/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

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

Why are we changing flags from global to local? It changes where we can place these flag in a command.

dot/network/host.go Outdated Show resolved Hide resolved
@edwardmack
Copy link
Member Author

Why are we changing flags from global to local? It changes where we can place these flag in a command.

This was done because we upgraded to v2 of urfave/cli and GlobalString, etc have been deprecated. https://cli.urfave.org/migrate-v1-to-v2/#globalstring-globalbool-and-its-likes-are-deprecated

I don't believe this changes where we can place these flags in a command.

@edwardmack edwardmack force-pushed the ed/cli_flags_update branch 2 times, most recently from 18d3436 to 8120862 Compare February 23, 2023 22:51
cmd/gossamer/config.go Outdated Show resolved Hide resolved
cmd/gossamer/config.go Outdated Show resolved Hide resolved
cmd/gossamer/config.go Show resolved Hide resolved
Copy link
Contributor

@timwu20 timwu20 left a comment

Choose a reason for hiding this comment

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

LGTM, just the one question.

cmd/gossamer/config.go Show resolved Hide resolved
Copy link

🎉 This PR is included in version 0.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

6 participants