Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Envoy to 12b3d2c2ffa582507e5d6dd34632b2b990f1b195 #376

Merged
merged 7 commits into from
Jun 22, 2020

Conversation

oschaaf
Copy link
Member

@oschaaf oschaaf commented Jun 19, 2020

Notable changes in this Update:

  • Changes related to header construction
  • CI moves from clang-9 to clang-10
  • Changes related to satisfy the new clang-tidy version:
    • Fix superfluous initializers in the rate limiter code.
    • Suppress modernize-avoid-bind to get sequencer_test.cc to pass checks.
      (clang-tidy's auto generated suggested fix fails to build)
  • Move to use typed configs in AddFilter() in the server filter tests.
    This avoids asserts being triggered by the integration test framework
    as deprecated fields for which support was dropped get generated
    otherwise.
  • Drop the coverage threshold by 0.1%

Hopefully addresses #362 by producing more reliable and accurate
coverage reports.

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com


prerequisite before merge

Has some mundane changes related to header construction.
Switches us to clang-10, which hopefully addresses envoyproxy#362

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added P1 waiting-for-review A PR waiting for a review. labels Jun 19, 2020
oschaaf added 2 commits June 20, 2020 00:31
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf removed the waiting-for-review A PR waiting for a review. label Jun 19, 2020
oschaaf added 3 commits June 20, 2020 10:47
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
- Fix superfluous initializers in the rate limiter code.
- Suppress modernize-avoid-bind in sequencer_test.cc.
  (clang-tidy's auto generated suggested fix fails to build)

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Need to analyse why this is needed yet.
Assuming everything is allright, dropping the threshold for now.

Potential causes:
- clang-tidy-10 is more accurate
- we dropped a few lines of code. If we borderlined. this induces
  a lower coverage percentage threshold requirement.
- something else.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added the waiting-for-review A PR waiting for a review. label Jun 20, 2020
@oschaaf
Copy link
Member Author

oschaaf commented Jun 20, 2020

The coverage threshold had to be lowered because of small changes, probably related to clang-10.

For example, compare the new factories impl coverage with the latest from master.
Both count these as missing coverage, but only the new version reports that the default switch label isn't hit:

(new)
     102           0 :   default:
     103           0 :     NOT_REACHED_GCOVR_EXCL_LINE;
(old)
     102          75 :   default:
     103           0 :     NOT_REACHED_GCOVR_EXCL_LINE;

I think that makes sense, and that the new version is more accurate. So lowering the coverage threshold seems
the right thing to do.

The attempt to do it more fine-grained failed.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@mum4k mum4k merged commit fae9b7c into envoyproxy:master Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 waiting-for-review A PR waiting for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants