Conversation
- Add DisableProtoValidate bool field to config (default: false) - Call interceptors.SetDisableProtoValidate() during processConfig - go.mod has replace directive removed — depends on interceptors#35 being merged first
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis change introduces a new configuration option Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new runtime configuration flag to optionally disable proto validation (protovalidate) in the default gRPC interceptor chain, wiring it through core startup config processing.
Changes:
- Add
DisableProtoValidatetoconfig.Configwith env varDISABLE_PROTO_VALIDATE(defaultfalse). - Wire the flag into startup via
interceptors.SetDisableProtoValidate(true)inprocessConfig. - Update Go module metadata (
go.mod/go.sum) with new transitive dependencies (protovalidate/CEL-related).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
config/config.go |
Introduces the new config field and envconfig tag for DISABLE_PROTO_VALIDATE. |
core.go |
Applies the new config flag to the interceptors package during config processing. |
go.mod |
Adds new indirect dependencies (protovalidate/CEL-related). |
go.sum |
Updates module checksums; notably drops interceptors checksums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if c.config.DisableProtoValidate { | ||
| interceptors.SetDisableProtoValidate(true) | ||
| } |
There was a problem hiding this comment.
The new DisableProtoValidate branch in processConfig is currently uncovered by tests. There are already non-parallel branch-coverage tests for processConfig in core_coverage_test.go; adding a small test that sets DisableProtoValidate=true and calls processConfig() would prevent regressions (especially given interceptors configuration is global state).
There was a problem hiding this comment.
Will add a test case in the follow-up commit.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require ( | ||
| 4d63.com/gocheckcompilerdirectives v1.3.0 // indirect | ||
| 4d63.com/gochecknoglobals v0.2.2 // indirect | ||
| buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go v1.36.11-20260209202127-80ab13bee0bf.1 // indirect | ||
| buf.build/go/protovalidate v1.1.3 // indirect | ||
| cel.dev/expr v0.25.1 // indirect |
There was a problem hiding this comment.
go.mod still requires github.com/go-coldbrew/interceptors v0.1.19, but this PR adds protovalidate/CEL-related indirect dependencies that appear to come from a newer interceptors version (per PR description dependency on interceptors#35). Bump interceptors to a version that includes the new protovalidate toggle API, then re-run go mod tidy so indirect requirements reflect the actual dependency graph.
There was a problem hiding this comment.
Expected — interceptors version will be bumped after interceptors#35 is merged and tagged. The indirect deps were pulled in during local testing with a replace directive (now removed).
… doc - Bump interceptors to v0.1.20 (protovalidate support) - Add DisableProtoValidate config field - Wire to interceptors.SetDisableProtoValidate in processConfig - Fix make doc: use explicit package list instead of ./... to skip benchmarks dir (separate Go module with only test files)
Summary
DisableProtoValidateconfig field (DISABLE_PROTO_VALIDATEenv var, default: false)interceptors.SetDisableProtoValidate()during processConfigDepends on: go-coldbrew/interceptors#35
Test plan
go test -race ./...passes (with local replace directive)Summary by CodeRabbit
Release Notes
New Features
DISABLE_PROTO_VALIDATEconfiguration option to disable protocol buffer validation in the interceptor chain.Chores