Remove unused CheckResponse.Error #906
Conversation
Codecov ReportBase: 37.54% // Head: 37.55% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #906 +/- ##
==========================================
+ Coverage 37.54% 37.55% +0.01%
==========================================
Files 286 286
Lines 20852 20825 -27
==========================================
- Hits 7828 7821 -7
+ Misses 12541 12522 -19
+ Partials 483 482 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @krdln)
api/aperture/flowcontrol/check/v1/check.proto line 39 at r1 (raw file):
google.protobuf.Timestamp end = 2; // services that matched repeated string services = 4;
Shouldn't we change numbering now? If not, shouldn't we somehow "reserve" the 3?
docs/content/assets/openapiv2/aperture.swagger.yaml line 235 at r1 (raw file):
default: REJECT_REASON_NONE description: RejectReason contains fields that give further information about rejection. ClassifierInfoError:
I don't understand why this change in docs happens
This field described only authz-specific errors and was filled in envoy.Handler.Check() response when also returning non-nil error, but in such case the grpc framework was not using the response anyway. This field was also used for metrics, but no codepath was actually setting them, as flowcontrol never set these. Also: * Create errors using grpc/status package, so that we can control the grpc status. * Add missing sampled logs for error conditions. Drive-by: * Remove unused error from ClassifierEngine.Classify(), as it's infallible (all errors are reported individually per-label). * Remove unused code from authz.go.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kwapik)
api/aperture/flowcontrol/check/v1/check.proto line 39 at r1 (raw file):
Previously, kwapik (Krzysztof Kwapisiewicz) wrote…
Shouldn't we change numbering now? If not, shouldn't we somehow "reserve" the 3?
Don't see any compelling reason on why to renumber. Even though we don't care too much about compatibility, it won't hurt being more compatible than less 🤷
We could reserve, but since we're still alpha, I don't see too much value in it.
docs/content/assets/openapiv2/aperture.swagger.yaml line 235 at r1 (raw file):
Previously, kwapik (Krzysztof Kwapisiewicz) wrote…
I don't understand why this change in docs happens
I don't think anyone does :P I guess that now there are less entities called Error and this affects autogenerated names?
krdln
force-pushed
the
authz-error-revamp
branch
from
ee2bfb3 to
f4b603f
Compare
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @krdln)
api/aperture/flowcontrol/check/v1/check.proto line 39 at r1 (raw file):
Previously, krdln (Michał Krasnoborski) wrote…
Don't see any compelling reason on why to renumber. Even though we don't care too much about compatibility, it won't hurt being more compatible than less 🤷
We could reserve, but since we're still alpha, I don't see too much value in it.
I don't have strong opinion on that, so let's just keep it as is.
This field described only authz-specific errors and was filled in
envoy.Handler.Check() response when also returning non-nil error, but in
such case the grpc framework was not using the response anyway.
This field was also used for metrics, but no codepath was actually
setting them, as flowcontrol never set these.
Also:
status.
Drive-by:
infallible (all errors are reported individually per-label).
Resolves #894
Checklist
This change is