Skip to content
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

process grpc header flag for exit #8334

Conversation

twoshark
Copy link
Contributor

@twoshark twoshark commented Jan 25, 2021

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

This PR enables processing of the grpc headers passed via flag to the account exit command

Which issues(s) does this PR fix?

Fixes #

#8333

Other notes for review

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2021

CLA assistant check
All committers have signed the CLA.

@nisdas
Copy link
Member

nisdas commented Jan 26, 2021

Hey @twoshark , thanks for the PR , unfortunately it is missing the new dependencies that have to be added into our build file.

 bazel run //:gazelle

@twoshark
Copy link
Contributor Author

Hey @twoshark , thanks for the PR , unfortunately it is missing the new dependencies that have to be added into our build file.

 bazel run //:gazelle

Hey! Thanks, I've run it and it updated: validator/accounts/BUILD.bazel. I got the following output:

➜ bazel run //:gazelle
WARNING: The following configs were expanded more than once: [blst_enabled]. For repeatable flags, repeats are counted twice and may lead to unexpected behavior.
DEBUG: /private/var/tmp/_bazel_tusharshah/3f45103db17b7158700e6ae8b56c98c1/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:100:14: rbe_ubuntu_clang_gen not using checked in configs as user set attr to 'False'
DEBUG: /private/var/tmp/_bazel_tusharshah/3f45103db17b7158700e6ae8b56c98c1/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:100:14: rbe_ubuntu_gcc_gen not using checked in configs as user set attr to 'False'
INFO: Analyzed target //:gazelle (114 packages loaded, 7773 targets configured).
INFO: Found 1 target...
Target //:gazelle up-to-date:
  bazel-bin/gazelle-runner.bash
  bazel-bin/gazelle
INFO: Elapsed time: 236.477s, Critical Path: 14.35s
INFO: 102 processes: 13 internal, 89 darwin-sandbox.
INFO: Build completed successfully, 102 total actions
INFO: Build completed successfully, 102 total actions

@@ -183,13 +184,22 @@ func prepareClients(cliCtx *cli.Context) (*ethpb.BeaconNodeValidatorClient, *eth
if dialOpts == nil {
return nil, nil, errors.New("failed to construct dial options")
}
for _, hdr := range strings.Split(cliCtx.String(flags.GrpcHeadersFlag.Name), ",") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. To accept and merge this PR, we are going to need a regression test. Do you mind adding one for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djoyahoy has added the test and the validator tests pass:

Target //validator:go_default_test up-to-date:
  bazel-bin/validator/go_default_test_/go_default_test
INFO: Elapsed time: 8.898s, Critical Path: 8.30s
INFO: 46 processes: 6 internal, 40 darwin-sandbox.
INFO: Build completed successfully, 46 total actions
//validator:go_default_test                                              PASSED in 1.2s
  WARNING: //validator:go_default_test: Test execution time (1.2s excluding execution overhead) outside of range for MODERATE tests. Consider setting timeout="short" or size="small".
INFO: Build completed successfully, 46 total actions

We are however, seeing a failure in the end to end tests that I can't pin down:

> bazel test //endtoend:go_default_test --define=ssz=minimal
...
=== CONT  TestEndToEnd_MinimalConfig
    helpers.go:126: Ending time: 2021-01-26 16:55:28.701779 +0000 UTC m=+472.829059328
--- FAIL: TestEndToEnd_MinimalConfig (472.79s)
...
    --- FAIL: TestEndToEnd_MinimalConfig/validators_participating_epoch_5 (0.00s)
...
FAIL
================================================================================
Target //endtoend:go_default_test up-to-date:
  bazel-bin/endtoend/go_default_test_/go_default_test
INFO: Elapsed time: 493.339s, Critical Path: 487.68s
INFO: 65 processes: 4 internal, 61 darwin-sandbox.
INFO: Build completed, 1 test FAILED, 65 total actions
//endtoend:go_default_test                                               FAILED in 1 out of 2 in 475.8s
  Stats over 2 runs: max = 475.8s, min = 295.1s, avg = 385.4s, dev = 90.4s
  /private/var/tmp/_bazel_tusharshah/3f45103db17b7158700e6ae8b56c98c1/execroot/prysm/bazel-out/darwin-fastbuild/testlogs/endtoend/go_default_test/shard_1_of_2/test.log

INFO: Build completed, 1 test FAILED, 65 total actions

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about E2E failing, it's a bit moody. Sometimes it just has to be restarted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants