feat(parser) error for all inconsistent services #4171
Merged
Conversation
This file contains 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
rainest
force-pushed
the
feat/log-all-inconsistent
branch
2 times, most recently
from
b5533b5
to
af0ea15
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4171 +/- ##
=====================================
Coverage 60.6% 60.6%
=====================================
Files 150 150
Lines 16843 16821 -22
=====================================
- Hits 10213 10203 -10
+ Misses 5984 5974 -10
+ Partials 646 644 -2
☔ View full report in Codecov by Sentry. |
Emit errors for every Service in a multi-Service backend when an annotation is inconsistent. Previously, the parser would arbitrarily choose one annotation value as the correct value and only log errors for Services with a different value. There was no basis for choosing this as the correct value, it was just the first observed value, and there was no indication which Service was the source of the value. Logging for each informs users of the complete set of Services.
Remove the error count check from TestPopulateServices. This test checks whether a virtual service is skipped entirely, not the reason why. While the only existing test case does generate an error per component Service, this may not be the case for future skip reasons. The error count for that case (annotation mismatch) is already tested independently in the TestDoK8sServicesMatchAnnotations test specific to that skip reason.
rainest
force-pushed
the
feat/log-all-inconsistent
branch
from
adda5c6
to
696729b
Compare
randmonkey
reviewed
Jun 14, 2023
pmalek
reviewed
Jun 14, 2023
rainest
force-pushed
the
feat/log-all-inconsistent
branch
from
abdf622
to
cfe7980
Compare
Use the apimachinery Set type instead of writing the map directly. Add comments and rename functions to clarify when we filter out interesting annotations. Remove old TODO.
rainest
force-pushed
the
feat/log-all-inconsistent
branch
from
cfe7980
to
32e7749
Compare
pmalek
approved these changes
Jun 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does / why we need it:
Emit errors for every Service in a multi-Service backend when an annotation is inconsistent.
Previously, the parser would arbitrarily choose one annotation value as the correct value and only log errors for Services with a different value. There was no basis for choosing this as the correct value, it was just the first observed value, and there was no indication which Service was the source of the value. Logging for each informs users of the complete set of Services.
Shortens the log to remove the "review the Route" string, as we're now listing all Services of interest.
Which issue this PR fixes:
Fix #3032
Special notes for your reviewer:
The original request in #3032 was already addressed by a previous change. We were already including Service information in these logs after we converted it to a translation error. However, we would only log Services with the "incorrect" value when there was no basis for choosing the "correct" value--it was just the first one we saw in the loop.
Users need to review all involved Services to select a correct value based on external information, so we want to include all of them.
I initially adjusted the check in TestPopulateServices rather than removing it, but on further review it looks like we shouldn't be checking error count there, only that the service is indeed skipped. Error count should instead be checked for tests of individual skip reasons (it is). Left as a separate commit for comparison to the initial fix.
PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR