livepeer_cli: use the O's currently registered Service URI as default address #2416
Conversation
emranemran
requested review from
thomshutt,
leszko,
AlexKordic,
yondonfu and
cyberj0g
Codecov Report
@@ Coverage Diff @@
## master #2416 +/- ##
===================================================
- Coverage 54.95519% 54.92276% -0.03244%
===================================================
Files 93 93
Lines 19414 19420 +6
===================================================
- Hits 10669 10666 -3
- Misses 8150 8159 +9
Partials 595 595
Continue to review full report at Codecov.
|
|
Tested locally with both off-chain and on-chain BOT nodes - it's working as expected and addresses the ticket's need. |
There was a problem hiding this comment.
Thanks for the PR @emranemran Added one comment.
Other than that, could you improve the PR description with the following points?
- Change
#2293tofix #2293. Then, merging this PR will automatically close the related GH Issue - Update
PENDING_CHANGELOG.md - Fill the checklist:
- Run
./testlocally and check it if it works - Cross out
README and other documentation updatedif there is README update related to this PR - Update
PENDING_CHANGELOG.mdand checkPending changelog updated
| @@ -66,13 +67,16 @@ func (w *wizard) promptOrchestratorConfig() (float64, float64, int, int, string) | |||
| fmt.Printf("Enter the price for %d pixels in Wei (required) ", pixelsPerUnit) | |||
| pricePerUnit := w.readDefaultInt(0) | |||
|
|
|||
| addr := myHostPort() | |||
| if w.offchain { | |||
There was a problem hiding this comment.
I think that this if does not make sense, because you can never run this function in the offchain mode. Orchestrator activation and setting orchestrator config is only for on-chain.
However, I think you need another condition here.
- If
orch.ServiceURIis already set (orchestrator is already registered on-chain), then useorch.ServiceURI. - Otherwise, use
myHostPort()
There was a problem hiding this comment.
Got it, that makes more sense. I was thinking there might be some use cases for off-chain where myHostPort needs is used as-is. Will update PR.
There was a problem hiding this comment.
Addressed all comments. Also added a condition to not allow orchestrator config settings to be changed if off-chain is detected based on your comments.
Only outstanding issue was that test.sh fails locally for unrelated reasons - likely related to clang compiler that gets used by default on my Mac M1. The tests executed in this PR all pass so this should be OK to merge. If it's alright, I can follow up with a fix for test.sh failures on Macs as part of a separate ticket.
/opt/homebrew/Cellar/ffmpeg/5.0.1/include/libavformat/avformat.h:2161:41: note: passing argument to parameter 'decoder_ret' here
extras.c:193:13: error: implicit declaration of function 'avfilter_compare_sign_bypath' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
extras.c:193:13: note: did you mean 'lpms_compare_sign_bypath'?
extras.c:191:5: note: 'lpms_compare_sign_bypath' declared here
extras.c:204:13: error: implicit declaration of function 'avfilter_compare_sign_bybuff' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
extras.c:204:13: note: did you mean 'lpms_compare_sign_bybuffer'?
extras.c:202:5: note: 'lpms_compare_sign_bybuffer' declared here
There was a problem hiding this comment.
If tests pass on GH Actions, that's fine. M1 may fail for some unrelated reasons. I also experience some test failures on M1.
emranemran
force-pushed
the
user/emahbub/use-serviceuri-cli
branch
2 times, most recently
from
7951465
to
5f304ed
Compare
| @@ -66,13 +67,16 @@ func (w *wizard) promptOrchestratorConfig() (float64, float64, int, int, string) | |||
| fmt.Printf("Enter the price for %d pixels in Wei (required) ", pixelsPerUnit) | |||
| pricePerUnit := w.readDefaultInt(0) | |||
|
|
|||
| addr := myHostPort() | |||
| if w.offchain { | |||
There was a problem hiding this comment.
If tests pass on GH Actions, that's fine. M1 may fail for some unrelated reasons. I also experience some test failures on M1.
When changing config options for the Orchestrator via livepeer_cli, the public host:port of the node is requested for the configuration to be applied successfully. This host:port address defaults to using the ip-address of the node instead of using the global domain name that is registered when setting up an O node. Use the registered Service URI as the default option such that the entry can be left blank when configuring options via the livepeer-cli. Update CHANGELOG_PENDING.md Co-authored-by: Rafał Leszko <[email protected]>
emranemran
force-pushed
the
user/emahbub/use-serviceuri-cli
branch
from
a8b1830
to
cbe02ed
Compare
When changing config options for the Orchestrator via livepeer_cli, the
public host:port of the node is requested for the configuration to be
applied successfully. This host:port address defaults to using the
ip-address of the node instead of using the global domain name that is
registered when setting up an O node. Use the registered Service URI as
the default option such that the entry can be left blank when
configuring options via the livepeer-cli.
What does this pull request do? Explain your changes. (required)
(see above commit desc)
Specific updates (required)
(see above commit desc)
How did you test each of these updates (required)
Does this pull request close any open issues?
fix #2293
Checklist:
makeruns successfully[ ] All tests in./test.shpass[ ] README and other documentation updated