-
Notifications
You must be signed in to change notification settings - Fork 33
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
[Client] [Tooling] refactor: CLI #806
Changes from 51 commits
34d42bf
142071d
bcf3f65
00730d4
41d18ae
507d8af
e74fea4
6128d84
4bc605a
094a95e
2a5b300
1d03f38
c1318da
8d1f62a
975acf3
e9a451f
72b7304
7924dda
df5eb7e
08bcafd
5a54400
5000ad8
c587afa
e70b99c
b3ce271
8a476a4
650d432
59fffb5
a1e22c4
ccd9fb7
6e1a5b8
60302a4
1ac2e00
3a8e138
0cbca43
9cee9dd
2489b92
11c61fc
4bf0bba
60ea916
3afcfa0
51d6f55
3bfa694
3d01244
e286cbf
82ad80a
093a78a
c8b05c3
3f42683
3faa07a
8c2b9f4
c98e8d1
84ce1bd
341d982
0b22fbd
a9ffa56
0d92a09
024d88b
22ff0fd
95c2d82
7b6e995
408a68d
c0577a3
044f6f1
b273e87
2e4d60d
14404da
c17bf87
a4de252
79c658d
413589f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package flags | ||
Olshansk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: I think adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, I was imagining browsing the godoc. Perhaps a package-level comment would be more appropriate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Adding this to my list of TODOs in a downstream PR to avoid another round with CI) |
||
RemoteCLIURL string | ||
Olshansk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// DataDir a path to store pocket related data (keybase etc.). | ||
// (see: --help the root command for more info). | ||
DataDir string | ||
|
||
// ConfigPath is the path to the node config file. | ||
// (see: --help the root command for more info). | ||
ConfigPath string | ||
|
||
// If true skips the interactive prompts wherever possible (useful for scripting & automation) | ||
// (see: --help the root command for more info). | ||
NonInteractive bool | ||
|
||
// Show verbose output | ||
// (see: --help the root command for more info). | ||
Verbose bool | ||
bryanchriswhite marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if it environment variable works? I love that you bind a viper configuration instead of looking up the environment variable!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so. The CLI client and cluster manager containers are using
POCKET_REMOTE_CLI_URL
in this branch and both k8s and docker-compose localnets seem to be working as expected. I even used a fresh kind cluster for the k8s localnet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 hmm... it doesn't seem to be working in the context of
make localnet_shell
though when it seems like it should, perhaps I've missed something. Taking another look. 👀There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okdas thanks for making me double check this. 😅
I did a bit of investigating and I'm noticing that the way
BindPFlag()
works is not quite what I expected. It seems to "bind the respective viper key" in the sense that the final result will be reflected in the value retrieved from viper (as opposed to updating the bound flag variable). 0b22fbd adds a line just after the call toParseConfig()
which updates the flag var to this final value. My thinking is that it's better to keep the flag var than to replace all references to it withviper.GetString("remote_cli_url")
or something similar.It's worth noting though that we'll have to do this explicitly for any other flag vars which we wish to express the same behavior.