Skip to content

options: splitting into base class and command line parsing#30924

Merged
alyssawilk merged 15 commits intoenvoyproxy:mainfrom
alyssawilk:tclap
Nov 29, 2023
Merged

options: splitting into base class and command line parsing#30924
alyssawilk merged 15 commits intoenvoyproxy:mainfrom
alyssawilk:tclap

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Nov 16, 2023

This allows removing the tclap dep from Envoy Mobile.

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #30924 was opened by alyssawilk.

see: more, trace.

@alyssawilk alyssawilk force-pushed the tclap branch 4 times, most recently from e9417f4 to da4bd51 Compare November 16, 2023 18:54
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Copy Markdown
Contributor Author

This is simply a code split with the exception of the new absl:::Status setLogLevel(absl::string_view) wrapper to avoid a bunch of boilerplate in E-M

Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for doing this and two comments are added.

If I got it correctly, this PR split previous OptionsImpl to OptionsImplBase to store all bootstrap options and new OptionsImpl to parse command line by the TCLAP. And and parsing part is unnecessary for the E-M, so we can use OptionsImplBase only in the E-M.

Comment thread source/server/options_impl.cc
Comment thread source/server/options_impl.cc
@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Nov 17, 2023

And please check the CI, thanks.

/wait

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

correct about the split. CI looks weird - a bunch of jobs randomly canceled. retesting but lmk if you want any changes!

Comment thread source/server/options_impl.cc
Comment thread source/server/options_impl.cc
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Copy Markdown
Contributor Author

oops there were 2 build fails buried in the cancels. on it!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Nov 17, 2023

oh, seems I cannot stamp it today before I go to 🛏️. CI still say no.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Nov 21, 2023

/retest

@alyssawilk
Copy link
Copy Markdown
Contributor Author

Azure Pipelines / envoy-presubmit (Linux x64 Build and test)
Started 19h 19m 57s ago
well that's not great.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Copy Markdown
Contributor Author

ugh I have no idea what's with the coverage failure and can't repro locally. I'll ping when this is ready for review.

@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Nov 22, 2023

🤣 Add waiting label first.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Copy Markdown
Contributor Author

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Copy Markdown
Contributor Author

/retest

@alyssawilk
Copy link
Copy Markdown
Contributor Author

FINALLY GREEN

@alyssawilk alyssawilk enabled auto-merge (squash) November 29, 2023 00:40
Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@alyssawilk alyssawilk merged commit 5c65602 into envoyproxy:main Nov 29, 2023
mum4k pushed a commit to envoyproxy/nighthawk that referenced this pull request Dec 4, 2023
- updated envoy commit number to 
- updated nighthawk common lib dependency: replace `envoy_common_lib_with_external_headers` with `main_common_lib_with_external_headers`  due to change envoyproxy/envoy#30924

Signed-off-by: fei-deng <feid@google.com>
eric846 pushed a commit to envoyproxy/nighthawk that referenced this pull request Dec 14, 2023
- change `context.runtime()` to `context.serverFactoryContext().runtime()` in source/server/http_dynamic_delay_filter_config.cc due to envoyproxy/envoy#31189 and implemented PANIC methods in NighthawkServerFactoryContext
- replace `@envoy//source/exe:envoy_common_lib_with_external_headers` with `@envoy//source/exe:all_extensions_lib_with_external_headers` due to envoyproxy/envoy#31109
- replace `@envoy//source/exe:main_common_lib_with_external_headers` with `@envoy//source/exe:main_common_with_all_extensions_lib_with_external_headers` due to a combination of envoyproxy/envoy#30924 and envoyproxy/envoy#31109 causing unit test `UtilityAddressResolutionTest, AddressResolution` failed because `envoy.network.dns_resolver.cares` was not linked in 
- lower `upstream_cx_rx_bytes_total` counter threshold in `test_http_h2` integration test.
- disable oghttp2 in Nighthawk which was enabled by default in Envoy in envoyproxy/envoy@b58d312
- sync files

Signed-off-by: Qin Qin <qqin@google.com>
@alyssawilk alyssawilk deleted the tclap branch March 19, 2024 19:46
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.

2 participants