runtime: load rtds bool correctly as true/false instead of 1/0#36044
Merged
alyssawilk merged 10 commits intoenvoyproxy:mainfrom Sep 11, 2024
Merged
runtime: load rtds bool correctly as true/false instead of 1/0#36044alyssawilk merged 10 commits intoenvoyproxy:mainfrom
alyssawilk merged 10 commits intoenvoyproxy:mainfrom
Conversation
|
Hi @gordon-wang-lyft, 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. |
c233209 to
9f47dbb
Compare
alyssawilk
reviewed
Sep 10, 2024
Contributor
alyssawilk
left a comment
There was a problem hiding this comment.
code LGTM but let's runtime guard just in case it causes problems for anyone.
/wait
| EXPECT_EQ("bar", loader_->snapshot().get("foo").value().get()); | ||
| EXPECT_EQ("yar", loader_->snapshot().get("bar").value().get()); | ||
| EXPECT_EQ("meh", loader_->snapshot().get("baz").value().get()); | ||
| EXPECT_EQ("true", loader_->snapshot().get("toggle").value().get()); |
Contributor
There was a problem hiding this comment.
just to confirm, this check fails prior to your change?
Contributor
Author
There was a problem hiding this comment.
Yes. This is a newly added check.
Without the source code change, it will fail, since snapshot().get("toggle").value().get() will return "1" instead of "true".
Contributor
Author
There was a problem hiding this comment.
Also updated a comment so it is more clear why absl::StrCat is not used for boolean case
9f47dbb to
44bfa84
Compare
44bfa84 to
ba5eb78
Compare
alyssawilk
reviewed
Sep 10, 2024
Contributor
alyssawilk
left a comment
There was a problem hiding this comment.
I do still think it's worth a runtime guard
/wait
Contributor
Author
|
/retest |
Contributor
Author
Will add and update soon. |
ba5eb78 to
b6e934f
Compare
9b6d4ac to
d1efd5b
Compare
Contributor
Author
Updated with runtime guard and specific test cases. Let me know how it looks like. Thanks @alyssawilk ! |
d1efd5b to
0b79a5b
Compare
…d of "0"/"1" Signed-off-by: Gordon Wang <gordonwang@lyft.com>
Signed-off-by: Gordon Wang <gordonwang@lyft.com>
Signed-off-by: Gordon Wang <gordonwang@lyft.com>
Signed-off-by: Gordon Wang <gordonwang@lyft.com>
Signed-off-by: Gordon Wang <gordonwang@lyft.com>
Signed-off-by: Gordon Wang <gordonwang@lyft.com>
0b79a5b to
ee84b3e
Compare
alyssawilk
approved these changes
Sep 11, 2024
unicell
added a commit
to unicell/envoy
that referenced
this pull request
Sep 11, 2024
* upstream/main: (21 commits) Add a CPU utilization resource monitor for overload manager (envoyproxy#34713) jwks: Add UA string to headers (envoyproxy#35977) exceptions: cleaning up macros (envoyproxy#35694) coverage: ratcheting (envoyproxy#36058) runtime: load rtds bool correctly as true/false instead of 1/0 (envoyproxy#36044) Typo in documentation of http original_src filter (envoyproxy#36060) docs: updating meeting info (envoyproxy#36052) quic: removes more references to spdy::Http2HeaderBlock. (envoyproxy#36057) json: add null support to the streamer (envoyproxy#36051) json: make the streamer a template class (envoyproxy#36001) docs: Add `apt.envoyproxy.io` install information (envoyproxy#36050) ext_proc: elide redundant copy in ext_proc filter factory callback (envoyproxy#36015) build(deps): bump yarl from 1.11.0 to 1.11.1 in /tools/base (envoyproxy#36049) build(deps): bump multidict from 6.0.5 to 6.1.0 in /tools/base (envoyproxy#36048) quic: enable certificate compression/decompression (envoyproxy#35999) Geoip fix asan failure (envoyproxy#36043) mobile: Fix missing logging output in Swift integration tests (envoyproxy#36040) http: minor code clean up to the http filter manager (envoyproxy#36027) ci/example: Dont build/test the filter example in Envoy CI (envoyproxy#36038) ci/codeql: Fix build setup (envoyproxy#36021) ... Signed-off-by: Qiu Yu <qiuyu@apple.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: Load RTDS boolean config correctly as true/false instead of 1/0
Additional Description: Currently, RTDS boolean values are being loaded as
1or0, which is inconsistent with the expectedtrueorfalsevalues. This discrepancy needs to be corrected so that boolean values are consistently loaded and represented astrueorfalse. For further details, please refer to the issue comment where this inconsistency is discussed.Risk Level: Low (to be consistent with initially loaded true/false value)
Testing: new Unit Test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
[Optional Fixes #Issue] #35762