fix: fill in the required args for Api::Impl #14554
Conversation
|
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. |
ynqa
changed the title
|
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 |
Signed-off-by: Makoto Ito <un.pensiero.vano@gmail.com>
| Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system, | ||
| platform_impl_.fileSystem()); | ||
| platform_impl_.fileSystem(), rand); |
There was a problem hiding this comment.
The compilation breakage should have been caught by the existing build rule. Do we not try to build everything under //tools somewhere in CI?
There was a problem hiding this comment.
We must not which seems broken. Can you add this somewhere here? https://github.com/envoyproxy/envoy/blob/master/ci/do_ci.sh
/wait
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
bazel build //tools/... should suffice.
There was a problem hiding this comment.
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/...
There was a problem hiding this comment.
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.
Signed-off-by: Makoto Ito <un.pensiero.vano@gmail.com>
| Envoy::Api::Impl api(platform_impl_.threadFactory(), stats_store, time_system, | ||
| platform_impl_.fileSystem()); | ||
| platform_impl_.fileSystem(), rand); |
There was a problem hiding this comment.
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/...
Signed-off-by: Antonio Vicente <avd@google.com>
There was a problem hiding this comment.
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! |
* 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>
Commit Message: Add
Random::RandomGenerator&as an argument which is required byApi::Impl.Risk Level: Low