[Client] [Tooling] refactor: CLI #806
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryanchriswhite
added
tooling
bryanchriswhite
force-pushed
the
refactor/cli
branch
from
bcc5e8c to
72b7304
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
==========================================
+ Coverage 31.37% 31.52% +0.15%
==========================================
Files 107 107
Lines 9085 9034 -51
==========================================
- Hits 2850 2848 -2
+ Misses 5895 5846 -49
Partials 340 340
☔ View full report in Codecov by Sentry. |
bryanchriswhite
force-pushed
the
refactor/cli
branch
from
3559a27 to
df5eb7e
Compare
bryanchriswhite
force-pushed
the
refactor/peerstore-provider
branch
from
0749b80 to
4589585
Compare
bryanchriswhite
force-pushed
the
refactor/peerstore-provider
branch
from
4589585 to
22ff0fd
Compare
* refactor/peerstore-provider: chore: update changelogs fix: p2p test chore: fix consensus test chore: persistence peerstore provider includes all staked actors refactor: rename `NewPersistencePeerstoreProvider()` to `Create()` chore: combine `NewRPCPeerstoreProvider()` & `Create()` chore: add TECHDEBT comments
bryanchriswhite
force-pushed
the
refactor/cli
branch
from
e225361 to
95c2d82
Compare
3 tasks
Olshansk
approved these changes
Jun 12, 2023
There was a problem hiding this comment.
@bryanchriswhite Aside from the failing E2E tests, this LGTM and I don't have much feedback to provide.
I know you still have to resolve some E2E failures but doing a "soft approval" in the meantime so we can get this over the finish line.
| @@ -43,6 +48,9 @@ var rootCmd = &cobra.Command{ | |||
| PersistentPreRunE: func(cmd *cobra.Command, args []string) error { | |||
| // by this time, the config path should be set | |||
| cfg = configs.ParseConfig(flags.ConfigPath) | |||
|
|
|||
| // set final `remote_cli_url` value; order of precedence: flag > env var > config > default | |||
There was a problem hiding this comment.
Should this be something we do for every flag?
There was a problem hiding this comment.
It would probably be a good idea, for consistency across the CLI UX, to do that, yea.
| @@ -1,13 +1,24 @@ | |||
| package flags | |||
|
|
|||
| var ( | |||
| // RemoveCLIURL is the URL of the remote RPC node which the CLI will interact with. | |||
| // Formatted as <protocol>://<host>:<port> (uses RPC Port). | |||
| // (see: --help the root command for more info). | |||
There was a problem hiding this comment.
NIT: I think adding (see: --help the root command for more info). once above var ( would be sufficient.
There was a problem hiding this comment.
Fair enough, I was imagining browsing the godoc. Perhaps a package-level comment would be more appropriate.
There was a problem hiding this comment.
(Adding this to my list of TODOs in a downstream PR to avoid another round with CI)
| "takes a remote endpoint in the form of <protocol>://<host>:<port> (uses RPC Port)", | ||
| ) | ||
|
|
||
| // ensure that the env var can override the flag |
There was a problem hiding this comment.
I know we've had a lot of issues with viper and flag binding in the past.
I don't have much feedback on this as I'd have to dive in really deep, but have to be honest that I have lost context on where/how this variable binding is working.
Anything I should focus on or provide feedback in particular? If not, LGTM
There was a problem hiding this comment.
What I think I've learned regarding bindings: cobra takes the default values and handles updating bound flag variables/pointers based on any actual flag values which are present during invocation (i.e. --remote_cli_url someValue). Separately from that, viper can track a key of the same name, the value of which (independent of the flag vars/ptrs) will default to its respective config value (if present/applicable) but can be overridden by a respective env var and/or the flag value.
TLDR; the viper/cobra API dynamic is such that:
- Viper key/values are separate (in memory) from viper flags.
- Viper key/values can be overridden by cobra flags but not the other way around.
- For consistency we should either:
- Ensure we're updating flag vars (i.e. bound to a cobra flag) with their respective viper overrides
- Stop using flag vars (e.g. do
#PersistentFlags.String(...)instead of#PersistentFlags.StringVar(&flagVar, ...)and treat viper as the single source of truth (e.g.viper.GetString(...))
bryanchriswhite
commented
Jun 13, 2023
(cherry picked from commit 5254967bbe067e30c02fb63d40bc263f3d6def22)
* refactor/peerstore-provider: chore: fix nit chore: update changelogs revert: `readCtx.GetAllStakedActors()`
bryanchriswhite
added a commit
that referenced
this pull request
Jun 13, 2023
## Description
1. Simplify `PeerstoreProvider` interface
2. Extend it to support retrieval of the unstaked actor peerstore
3. Implement the extended interface in `persistencePeerstoreProvider`
### Before
```mermaid
classDiagram
class InterruptableModule {
<<interface>>
Start() error
Stop() error
}
class IntegratableModule {
<<interface>>
+SetBus(bus Bus)
+GetBus() Bus
}
class InitializableModule {
<<interface>>
+GetModuleName() string
+Create(bus Bus, opts ...Option) (Module, error)
}
class Module {
<<interface>>
}
Module --|> InitializableModule
Module --|> IntegratableModule
Module --|> InterruptableModule
class PeerstoreProvider {
<<interface>>
+GetStakedPeerstoreAtHeight(height int) (Peerstore, error)
+GetP2PConfig() *P2PConfig
}
class persistencePeerstoreProvider
class rpcPeerstoreProvider
persistencePeerstoreProvider --|> PeerstoreProvider
rpcPeerstoreProvider --|> PeerstoreProvider
PeerstoreProvider --|> Module
```
### After
```mermaid
classDiagram
class IntegratableModule {
<<interface>>
+GetBus() Bus
+SetBus(bus Bus)
}
class PeerstoreProvider {
<<interface>>
+GetStakedPeerstoreAtHeight(height int) (Peerstore, error)
+GetUnstakedPeerstore() (Peerstore, error)
}
class persistencePeerstoreProvider
class rpcPeerstoreProvider
class p2pModule
class unstakedPeerstoreProvider {
<<interface>>
+GetUnstakedPeerstore() (Peerstore, error)
}
persistencePeerstoreProvider --|> PeerstoreProvider
persistencePeerstoreProvider --> p2pModule : from Bus
rpcPeerstoreProvider --> p2pModule : from Bus
p2pModule --|> unstakedPeerstoreProvider
rpcPeerstoreProvider --|> PeerstoreProvider
rpcPeerstoreProvider --|> Module
PeerstoreProvider --|> IntegratableModule
class Module {
<<interface>>
}
Module --|> InitializableModule
Module --|> IntegratableModule
Module --|> InterruptableModule
```
## Issue
Realted:
- #810
Dependants:
- #505
- #806
## Type of change
Please mark the relevant option(s):
- [ ] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->
## List of changes
- Replaced embedded `modules.Module` with simpler
`modules.IntegratableModule` in `PeerstoreProvider` interface
- Removed unused `PeerstoreProvider#GetP2PConfig()` method
- Added `PeerstoreProvider#GetUnstakedPeerstore()` method
- Added `p2pPeerstoreProvider` implementation of `PeerstoreProvider`
interface
- Added `Factory` generic type
## Testing
- [ ] `make develop_test`; if any code changes were made
- [x] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [x] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [x] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made
<!-- REMOVE this comment block after following the instructions
If you added additional tests or infrastructure, describe it here.
Bonus points for images and videos or gifs.
-->
## Required Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG
### If Applicable Checklist
- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
bryanchriswhite
force-pushed
the
refactor/cli
branch
from
9d608ec to
79c658d
Compare
bryanchriswhite
changed the base branch from
refactor/peerstore-provider
to
main
bryanchriswhite
removed
do not merge
This comment was marked as outdated.
This comment was marked as outdated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Summary generated by Reviewpad on 13 Jun 23 12:53 UTC
This pull request includes various updates across the codebase, including changes to CLI flags, import statements, and default values. It also involves refactoring code to use shared helper functions, removing unused code, and improving error handling. Some variables have been renamed to be more descriptive. The specific changes include modifications to files such as
validator.go,cluster-manager.yaml,cli-client.yaml,utils.go, andruntime_defaults.go. For example, theRPC_HOSTvariable has been replaced withPOCKET_REMOTE_CLI_URL. A newhelperpackage has been added, and a newflagspackage contains several CLI flags. Code has also been updated to useflags.RemoteCLIURLinstead ofremoteCLIURL. Some constants have been renamed to includeHostname, and there are updates to endpoint hostnames for some validators.Issue
Dependants:
Type of change
Please mark the relevant option(s):
List of changes
rootCmdto support cross-package usage (i.e. subcommands w/ own pkgs)PersistentPreRunE()helperbusCLICtxKeyGetValueFromCLIContext()andSetValueInCLIContext()setupAndStartP2PModulesetupCurrentHeightProvidersetupPeerstoreProviderremote-cli-urlflag to cluster manager & refactorremote-cli-urlflag with env varRandomValidatorEndpointK8SHostnameconstantTesting
make develop_test; if any code changes were mademake test_e2eon k8s LocalNet; if any code changes were madee2e-devnet-testpasses tests on DevNet; if any code was changedRequired Checklist
godocformat comments on touched members (see: tip.golang.org/doc/comment)If Applicable Checklist
shared/docs/*if I updatedshared/*README(s)