-
Notifications
You must be signed in to change notification settings - Fork 167
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
cli: Show O rather than B options when -redeemer flag set #2456
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2456 +/- ##
===================================================
- Coverage 54.86667% 54.84901% -0.01766%
===================================================
Files 94 94
Lines 19726 19736 +10
===================================================
+ Hits 10823 10825 +2
- Misses 8306 8313 +7
- Partials 597 598 +1
Continue to review full report at Codecov.
|
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.
Added one comment, other than that LGTM.
Also, just a note on the process. You already closed the related GH Issue #2257. However I think you should:
- Keep the GH Issue open until this PR is merged
- Comment in the PR Description
fix #2257
instead of#2257
(then the GH Issue will be automatically closed when the PR is merged)
cmd/livepeer_cli/livepeer_cli.go
Outdated
@@ -120,12 +120,14 @@ func (w *wizard) initializeOptions() []wizardOpt { | |||
} | |||
|
|||
func (w *wizard) filterOptions(options []wizardOpt) []wizardOpt { | |||
isOrchestratorOrRedeemer := w.orchestrator || w.isRedeemer() |
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 think that for the sake of consistency, I'd not execute w.isRedeemer()
(and therefore an HTTP call) here, but rather in the place where wizard
is initialized.
w.orchestrator = w.isOrchestrator() |
Also, an idea, maybe we should just rename w.orchestrator
=> w.orchestratorOrRedeemer
?
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.
Thanks @leszko, I hadn't realised about the fix #commit
flow, will do that next time.
I've moved the isRedeemer
call as suggested, but for the rename there are some places that w.orchestrator
is used where I wasn't sure if it made sense to also do it for a redeemer node: https://github.com/livepeer/go-livepeer/blob/master/cmd/livepeer_cli/livepeer_cli.go#L85
If you think it makes sense to include redeemers there, then I'll update to w.orchestratorOrRedeemer
as suggested
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.
That's fine what you did. Let's keep it as you coded it! 👍
What does this pull request do? Explain your changes. (required)
The CLI now shows Orchestrator rather than Broadcaster options when go-livepeer is running with the
-redeemer
flag set.Specific updates (required)
How did you test each of these updates (required)
Ran the app with the
-redeemer
flag and the CLI before and after my changeDoes this pull request close any open issues?
fix #2257
Checklist:
make
runs successfully./test.sh
pass