Skip to content

fix: fill in the required args for Api::Impl#14554

Merged
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
ynqa:patch-1
Jan 10, 2021
Merged

fix: fill in the required args for Api::Impl#14554
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
ynqa:patch-1

Conversation

@ynqa
Copy link
Contributor

@ynqa ynqa commented Jan 3, 2021 •
edited
Loading

Commit Message: Add Random::RandomGenerator& as an argument which is required by Api::Impl.
Risk Level: Low

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Jan 3, 2021

Hi @ynqa, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14554 was opened by ynqa.

see: more, trace.

@ynqa ynqa changed the title fix: fill in the required args for Api::Impl. fix: fill in the required args for Api::Impl Jan 3, 2021
@rojkov
Copy link
Member

rojkov commented Jan 4, 2021

Thanks! Could you please amend the commit message to include a Signed-Off-By field? Otherwise the DCO check is bound to fail.

Also run ./tools/code_format/check_format.py fix to fix formatting errors.

@ynqa
include random generator
5f83c45 
Signed-off-by: Makoto Ito <un.pensiero.vano@gmail.com>
antoniovicente
antoniovicente reviewed Jan 6, 2021
View reviewed changes
tools/bootstrap2pb.cc
Envoy::Random::RandomGeneratorImpl rand;
Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system,
platform_impl_.fileSystem());
platform_impl_.fileSystem(), rand);
Copy link
Contributor

@antoniovicente antoniovicente Jan 6, 2021

Choose a reason for hiding this comment

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

cc @akonradi @lizan @htuch

I wonder if it would be possible to have a basic test the exercises this tool and catches compile breakages like this one.

Copy link
Contributor

@akonradi akonradi Jan 6, 2021

Choose a reason for hiding this comment

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

The compilation breakage should have been caught by the existing build rule. Do we not try to build everything under //tools somewhere in CI?

Copy link
Member

@mattklein123 mattklein123 Jan 6, 2021

Choose a reason for hiding this comment

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

We must not which seems broken. Can you add this somewhere here? https://github.com/envoyproxy/envoy/blob/master/ci/do_ci.sh

/wait

Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021

Choose a reason for hiding this comment

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

My understanding is that bazel test does not build binaries that are not required to run tests. In order to verify compilation of a binary, you need a test that depends on it.

Copy link
Member

@htuch htuch Jan 8, 2021

Choose a reason for hiding this comment

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

bazel build //tools/... should suffice.

Copy link
Member

@mattklein123 mattklein123 Jan 8, 2021

Choose a reason for hiding this comment

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

Yeah ^

Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the way to do this would be to change the following line in ci/build_setup.sh to also cover //tools/...
I can offer to send that as a separate change if we prefer just getting this build fix in and address the CI coverage issue separately.

[ -z "${ENVOY_BUILD_TARGET}" ] && export ENVOY_BUILD_TARGET=//source/exe:envoy-static //tools/...

Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021

Choose a reason for hiding this comment

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

There are other build breakages under //tools/... like:

ERROR: tools/testdata/protoxform/envoy/v2/BUILD:23:14: //tools/testdata/protoxform/envoy/v2:freeze_protos: missing input file '//tools/testdata/protoxform/envoy/v2:active_non_terminal.proto'
ERROR: tools/testdata/protoxform/envoy/v2/BUILD:23:14 3 input file(s) do not exist

I'm looking into them.

@ynqa
clang-format: change the order for includes
eead850 
Signed-off-by: Makoto Ito <un.pensiero.vano@gmail.com>
@ynqa ynqa marked this pull request as ready for review January 6, 2021 17:42
antoniovicente
antoniovicente approved these changes Jan 8, 2021
View reviewed changes
tools/bootstrap2pb.cc
Envoy::Random::RandomGeneratorImpl rand;
Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system,
platform_impl_.fileSystem());
platform_impl_.fileSystem(), rand);
Copy link
Contributor

@antoniovicente antoniovicente Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the way to do this would be to change the following line in ci/build_setup.sh to also cover //tools/...
I can offer to send that as a separate change if we prefer just getting this build fix in and address the CI coverage issue separately.

[ -z "${ENVOY_BUILD_TARGET}" ] && export ENVOY_BUILD_TARGET=//source/exe:envoy-static //tools/...

antoniovicente added a commit to antoniovicente/envoy that referenced this pull request Jan 9, 2021
@antoniovicente
bring in changes from PR envoyproxy#14554
80b3ff5 
Signed-off-by: Antonio Vicente <avd@google.com>
mattklein123
mattklein123 approved these changes Jan 10, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

As long as @antoniovicente (or someone) is working on testing tools in CI I'm fine to merge this as is. Thank you!

@mattklein123 mattklein123 merged commit abf8cd8 into envoyproxy:master Jan 10, 2021
@antoniovicente
Copy link
Contributor

antoniovicente commented Jan 11, 2021

As long as @antoniovicente (or someone) is working on testing tools in CI I'm fine to merge this as is. Thank you!

Yes, I'm looking into it in #14615

Thanks for the fix!

mpuncel added a commit to mpuncel/envoy that referenced this pull request Jan 12, 2021
@mpuncel
Merge branch 'master' into mpuncel/http2-hc-goaway
64cc217 
* master:
  http: support creating filters with match tree (envoyproxy#14430)
  [tls] Expose ServerContextImpl::selectTlsContext (envoyproxy#14592)
  docs: update ext_proc docs to reflect implementation status (envoyproxy#14636)
  filter manager: drop assert (envoyproxy#14633)
  kick off v1.18.0 (envoyproxy#14637)
  1.17.0 release (envoyproxy#14624)
  Implement request header processing in ext_proc (envoyproxy#14385)
  http: expose encoded headers/trailers via callbacks (envoyproxy#14544)
  [fuzz] fix minor fuzz bugs (envoyproxy#14593)
  rate limit: add computed descriptors (envoyproxy#14448)
  tools: fill in the required args for Api::Impl (envoyproxy#14554)
  Bump envoy-build to current images (envoyproxy#14608)

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+3 more reviewers

@akonradi akonradi akonradi left review comments

@htuch htuch htuch left review comments

@antoniovicente antoniovicente antoniovicente approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@antoniovicente antoniovicente

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ynqa @rojkov @antoniovicente @akonradi @mattklein123 @htuch