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

Revert "bazel: update to 6.0.0rc1 (#23678)" #24093

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

phlax
Copy link
Member

@phlax phlax commented Nov 20, 2022

Fix #23985

This reverts commit 84df237.

Signed-off-by: Ryan Northey ryan@synca.io

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:]

This reverts commit 84df237.

Signed-off-by: Ryan Northey <ryan@synca.io>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Nov 20, 2022
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #24093 was opened by phlax.

see: more, trace.

@phlax phlax marked this pull request as draft November 20, 2022 09:56
@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

fixing the coverage here exposes that the coverage has not been checked recently and is therefore not matching current per-file expectations

i was about to adjust the coverage when i noticed that one is off by a lot

Code coverage for source/extensions/clusters/common is lower than limit of 96.6 (68.2)
Code coverage for source/extensions/filters/http/cache/simple_http_cache is lower than limit of 96.0 (95.9)
Code coverage for source/extensions/filters/network/thrift_proxy/filters/payload_to_metadata is lower than limit of 96.6 (96.2)
Code coverage for source/server/admin is lower than limit of 97.5 (97.4)

@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

cc @alyssawilk

@phlax phlax changed the title [TESTING/WIP] Revert "bazel: update to 6.0.0rc1 (#23678)" Revert "bazel: update to 6.0.0rc1 (#23678)" Nov 21, 2022
@phlax phlax marked this pull request as ready for review November 21, 2022 08:42
@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

cc @keith

@RyanTheOptimist
Copy link
Contributor

This PR is marked as draft. Are you still working on it, or is it ready for review?

keith
keith previously approved these changes Nov 21, 2022
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

Fine to merge and I can debug after the holidays

@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

This PR is marked as draft.

it was - i think i already deWIPPed it

Are you still working on it, or is it ready for review?

it needs the per-file coverage to be adjusted - i can do that but in the case of the clusters code this is a big drop

Code coverage for source/extensions/clusters/common is lower than limit of 96.6 (68.2)

i guess we want to just adjust coverage and fix that separately - ill update

@RyanTheOptimist
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Nov 21, 2022
Signed-off-by: Ryan Northey <ryan@synca.io>
@keith
Copy link
Member

keith commented Nov 21, 2022

I think the solve for this might be this flag:

--experimental_fetch_all_coverage_outputs

@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

let me give that a whirl on a test PR ...

@phlax phlax changed the title Revert "bazel: update to 6.0.0rc1 (#23678)" [WIP] Revert "bazel: update to 6.0.0rc1 (#23678)" Nov 21, 2022
@phlax phlax marked this pull request as draft November 21, 2022 15:21
@keith
Copy link
Member

keith commented Nov 21, 2022

Guess we should also make this stuff fail when it breaks

@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

that doesnt seem to have worked

-r-xr-xr-x 1 envoybuild envoygroup    0 Nov 21 18:07 **_coverage_report.dat**

https://dev.azure.com/cncf/envoy/_build/results?buildId=121183&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881&l=2478

@phlax phlax marked this pull request as ready for review November 21, 2022 20:47
@phlax phlax changed the title [WIP] Revert "bazel: update to 6.0.0rc1 (#23678)" Revert "bazel: update to 6.0.0rc1 (#23678)" Nov 21, 2022
@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

i reckon lets land this for now and address the bazel/update/coverage issue separately

@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

dont-break-coverage ticket is here #24119

@phlax
Copy link
Member Author

phlax commented Nov 21, 2022

missing-coverage ticket #24120

@phlax phlax merged commit dede92e into envoyproxy:main Nov 21, 2022
dubious90 pushed a commit to envoyproxy/nighthawk that referenced this pull request Nov 28, 2022
- Update the ENVOY_COMMIT and ENVOY_SHA in [bazel/repositories.bzl](https://github.com/envoyproxy/nighthawk/blob/main/bazel/repositories.bzl) to the latest Envoy's commit.
- Update [.bazelrc](https://github.com/envoyproxy/nighthawk/blob/main/.bazelrc) to match Envoy commit envoyproxy/envoy#24093
- Update [.bazelversion](https://github.com/envoyproxy/nighthawk/blob/main/.bazelversion) to match Envoy commit envoyproxy/envoy#24093
- Update [ci/run_envoy_docker.sh](https://github.com/envoyproxy/nighthawk/blob/main/ci/run_envoy_docker.sh) to match Envoy commit envoyproxy/envoy#24107

Signed-off-by: tomjzzhang <4367421+tomjzzhang@users.noreply.github.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.

CI: Coverage is currently broken
4 participants