Skip to content

Ecds config dump recommit#24384

Merged
yanavlasov merged 2 commits intoenvoyproxy:mainfrom
yanjunxiang-google:ecds_config_dump_recommit
Dec 7, 2022
Merged

Ecds config dump recommit#24384
yanavlasov merged 2 commits intoenvoyproxy:mainfrom
yanjunxiang-google:ecds_config_dump_recommit

Conversation

@yanjunxiang-google
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google commented Dec 6, 2022

ECDS config dump is initially committed by #23902 .
#23902 is later on reverted by #24354 due to a few lines of code is not tested #24362.

This PR reverted #24354 to reinstall #23902 also added the changes of #24362 in.

Please check the original code review history of #23902 for details.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

…verted by (envoyproxy#24354)

This reverts commit c5d6160.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
…ment lines.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #24384 was opened by yanjunxiang-google.

see: more, trace.

@yanjunxiang-google
Copy link
Contributor Author

/assign @adisuissa @kyessenov @yanavlasov @alyssawilk

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

/lgtm api

Removing lines of comments to fix coverage seems to be the wrong thing to do, so I'd personally say that reducing the coverage threshold is better. Leaving it up to the senior maintainers to weigh in.
Other than that, LGTM!

@repokitteh-read-only repokitteh-read-only bot removed the api label Dec 6, 2022
@yanjunxiang-google
Copy link
Contributor Author

yanjunxiang-google commented Dec 6, 2022

/lgtm api

Removing lines of comments to fix coverage seems to be the wrong thing to do, so I'd personally say that reducing the coverage threshold is better. Leaving it up to the senior maintainers to weigh in. Other than that, LGTM!

Agreed. I think we should modify the CI scripts to fix this line-coverage counting issue on comments.
I removed the comments anyway since those two lines of comments are a little bit unnecessary since the code tells it all.

@kyessenov kyessenov self-requested a review December 6, 2022 19:58
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Coverage looks fine now.

@yanavlasov
Copy link
Contributor

Thanks for taking care of the coverage.

@yanavlasov yanavlasov merged commit df2f00a into envoyproxy:main Dec 7, 2022
jpsim added a commit that referenced this pull request Dec 7, 2022
….h-into-multiple-header-files

* origin/main:
  Add setRequestDecoder to ResponseEncoder interface (#24368)
  downstream: refactoring code to remove listener hard deps (#24394)
  lb api: moving load balancing policy specific configuration to extension configuration (#23967)
  ci: Skip docker/examples verification for docs or mobile only changes (#24417)
  ci: run mobile GitHub Actions on every PR (#24407)
  mobile: remove `bump_lyft_support_rotation.sh` script (#24404)
  Add file size to DirectoryEntry (#24176)
  bazel: update to 6.0.0rc4 (#24235)
  bazel: update rules_rust (#24409)
  Ecds config dump recommit (#24384)
  bazel: add another config_setting incompatible flag (#24270)
  listeners: moving listeners to extension directory (#24248)
  mobile: build Swift with whole module optimization (#24396)
  ci: update `actions/setup-java` from v1 to v3.8 (#24393)

Signed-off-by: JP Simard <jp@jpsim.com>
@yanjunxiang-google
Copy link
Contributor Author

Thanks for taking care of the coverage.

Thanks for approval!

jpsim added a commit that referenced this pull request Dec 8, 2022
…-cpp-to-latest-version

* origin/main: (23 commits)
  Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327)
  build: fix compile error for mac (#24429)
  postgres: support for upstream SSL (#23990)
  iOS: split `EnvoyEngine.h` into multiple header files (#24397)
  mobile: check for pending exceptions after JNI call (#24361)
  Remove uneccessary `this->` from mobile engine builder (#24389)
  Add setRequestDecoder to ResponseEncoder interface (#24368)
  downstream: refactoring code to remove listener hard deps (#24394)
  lb api: moving load balancing policy specific configuration to extension configuration (#23967)
  ci: Skip docker/examples verification for docs or mobile only changes (#24417)
  ci: run mobile GitHub Actions on every PR (#24407)
  mobile: remove `bump_lyft_support_rotation.sh` script (#24404)
  Add file size to DirectoryEntry (#24176)
  bazel: update to 6.0.0rc4 (#24235)
  bazel: update rules_rust (#24409)
  Ecds config dump recommit (#24384)
  bazel: add another config_setting incompatible flag (#24270)
  listeners: moving listeners to extension directory (#24248)
  mobile: build Swift with whole module optimization (#24396)
  ci: update `actions/setup-java` from v1 to v3.8 (#24393)
  ...

Signed-off-by: JP Simard <jp@jpsim.com>
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.

5 participants