[Merged by Bors] - require http and metrics for respective flags #4674

jxs · 2023-08-28T20:50:54Z

Issue Addressed

following discussion on #4639 (comment) this PR makes the http and metrics sub-flags to require those main flags enabled

default is false

jimmygchen

LGTM!

jimmygchen

Oh sorry, just realized we should probably make the same changes to beacon_node/src/cli.rs?

jxs · 2023-09-01T14:29:09Z

Oh sorry, just realized we should probably make the same changes to beacon_node/src/cli.rs?

yup makes sense Jimmy, updated!

jimmygchen · 2023-09-04T05:15:27Z

beacon_node/src/cli.rs

@@ -316,22 +316,25 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
        .arg(
            Arg::with_name("http-address")
                .long("http-address")
+                .requires("http")


Hey sorry @jxs, I've just realized that http api can be enabled via --staking or --gui as well, so --http is not strictly necessary, so this may be a breaking change for the beacon node, and we might want to leave this unchnaged?

lighthouse/beacon_node/src/config.rs

Lines 88 to 91 in d61f507

if cli_args.is_present("staking") {

client_config.http_api.enabled = true;

client_config.sync_eth1_chain = true;

}

lighthouse/beacon_node/src/config.rs

Lines 810 to 813 in d61f507

if cli_args.is_present("gui") {

client_config.http_api.enabled = true;

client_config.validator_monitor_auto = true;

}

btw I discovered this from the failing doppelganger tests, it uses --staking instead of --http

thanks for digging it Jimmy! I updated with a ArgGroup for http, gui and staking, when one of them is missing the following error is displayed:

error: The following required arguments were not provided: <--http|--gui|--staking>

Do you think it's clear enough? If not I can revert, meanwhile the doppelganger tests are still failing, can you access the logs? I am able to run the beacon_node for them locally but get a jwt validation error instead

ok managed to fix the doppelganger protection tests and it seems to me it's mo longer a breaking change since all the flags still work as only one of the three is required, but ptal Jimmy

michaelsproul · 2023-09-04T23:05:43Z

Marking this as backwards-incompat. We shouldn't merge this for v4.4.2, and should wait for v4.5.0.

to both add default values and then require them, fixes doppelganger protection test

michaelsproul

Nice! Let's merge

paulhauner · 2023-09-22T01:21:06Z

bors r+

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors · 2023-09-22T01:44:51Z

Build failed (retrying...):

release-tests-ubuntu

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors · 2023-09-22T01:58:58Z

Build failed (retrying...):

release-tests-ubuntu

michaelsproul · 2023-09-22T02:00:34Z

bors r-
bors r+

bors · 2023-09-22T02:00:48Z

Canceled.

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors · 2023-09-22T02:14:52Z

This PR was included in a batch that was canceled, it will be automatically retried

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors · 2023-09-22T03:24:50Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

## Issue Addressed Fixes breaking change introduced on #4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

## Issue Addressed following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

## Issue Addressed Fixes breaking change introduced on sigp#4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

## Issue Addressed following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

## Issue Addressed Fixes breaking change introduced on sigp#4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

## Issue Addressed following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

## Issue Addressed Fixes breaking change introduced on sigp#4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

jxs added 3 commits August 28, 2023 21:50

require http and metrics for respective flags

47acde7

update default to default_if to comply with requires

151d60f

remove unrequired required flags

bed073f

default is false

jimmygchen added the ready-for-review The code is ready for review label Aug 31, 2023

jxs requested a review from jimmygchen August 31, 2023 11:16

jimmygchen approved these changes Aug 31, 2023

View reviewed changes

jimmygchen removed the ready-for-review The code is ready for review label Aug 31, 2023

jimmygchen requested changes Aug 31, 2023

View reviewed changes

jimmygchen added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Aug 31, 2023

require http flag for http subflags for beacon node

5abe243

jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 4, 2023

jimmygchen reviewed Sep 4, 2023

View reviewed changes

make enable_http a group with http, gui, and staking

22e6177

michaelsproul added the backwards-incompat Backwards-incompatible API change label Sep 4, 2023

michaelsproul added the v4.5.0 ETA Q4 2023 label Sep 4, 2023

check for enable_http arggroup instead of http,

ce849eb

to both add default values and then require them, fixes doppelganger protection test

michaelsproul approved these changes Sep 22, 2023

View reviewed changes

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 22, 2023

bors bot pushed a commit that referenced this pull request Sep 22, 2023

require http and metrics for respective flags (#4674)

aa9a882

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors bot pushed a commit that referenced this pull request Sep 22, 2023

require http and metrics for respective flags (#4674)

46ba930

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors bot pushed a commit that referenced this pull request Sep 22, 2023

require http and metrics for respective flags (#4674)

a899b44

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors bot pushed a commit that referenced this pull request Sep 22, 2023

require http and metrics for respective flags (#4674)

c5588eb

## Issue Addressed following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled

bors bot changed the title ~~require http and metrics for respective flags~~ [Merged by Bors] - require http and metrics for respective flags Sep 22, 2023

bors bot closed this Sep 22, 2023

jxs mentioned this pull request Sep 22, 2023

[Merged by Bors] - Fix cli options #4772

Closed

bors bot pushed a commit that referenced this pull request Sep 22, 2023

Fix cli options (#4772)

e01e27f

## Issue Addressed Fixes breaking change introduced on #4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

bors bot pushed a commit that referenced this pull request Sep 22, 2023

Fix cli options (#4772)

710ddf8

## Issue Addressed Fixes breaking change introduced on #4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

bors bot pushed a commit that referenced this pull request Sep 22, 2023

Fix cli options (#4772)

0f05499

## Issue Addressed Fixes breaking change introduced on #4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

jxs deleted the require-http-metrics-flags branch September 24, 2023 12:24

Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024

Fix cli options (sigp#4772)

45dda5d

## Issue Addressed Fixes breaking change introduced on sigp#4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024

Fix cli options (sigp#4772)

30876ec

## Issue Addressed Fixes breaking change introduced on sigp#4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024

Fix cli options (sigp#4772)

b57b577

## Issue Addressed Fixes breaking change introduced on sigp#4674 that doesn't allow multiple `http_enabled` `ArgGroup` flags

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - require http and metrics for respective flags #4674

[Merged by Bors] - require http and metrics for respective flags #4674

jxs commented Aug 28, 2023

jimmygchen left a comment

jimmygchen left a comment

jxs commented Sep 1, 2023

jimmygchen Sep 4, 2023

jimmygchen Sep 4, 2023

jxs Sep 4, 2023

jxs Sep 8, 2023

michaelsproul commented Sep 4, 2023

michaelsproul left a comment

paulhauner commented Sep 22, 2023

bors bot commented Sep 22, 2023

bors bot commented Sep 22, 2023

michaelsproul commented Sep 22, 2023

bors bot commented Sep 22, 2023

bors bot commented Sep 22, 2023

bors bot commented Sep 22, 2023

	if cli_args.is_present("staking") {
	client_config.http_api.enabled = true;
	client_config.sync_eth1_chain = true;
	}

	if cli_args.is_present("gui") {
	client_config.http_api.enabled = true;
	client_config.validator_monitor_auto = true;
	}

[Merged by Bors] - require http and metrics for respective flags #4674

[Merged by Bors] - require http and metrics for respective flags #4674

Conversation

jxs commented Aug 28, 2023

Issue Addressed

jimmygchen left a comment

Choose a reason for hiding this comment

jimmygchen left a comment

Choose a reason for hiding this comment

jxs commented Sep 1, 2023

jimmygchen Sep 4, 2023

Choose a reason for hiding this comment

jimmygchen Sep 4, 2023

Choose a reason for hiding this comment

jxs Sep 4, 2023

Choose a reason for hiding this comment

jxs Sep 8, 2023

Choose a reason for hiding this comment

michaelsproul commented Sep 4, 2023

michaelsproul left a comment

Choose a reason for hiding this comment

paulhauner commented Sep 22, 2023

bors bot commented Sep 22, 2023

bors bot commented Sep 22, 2023

michaelsproul commented Sep 22, 2023

bors bot commented Sep 22, 2023

bors bot commented Sep 22, 2023

bors bot commented Sep 22, 2023