-
Notifications
You must be signed in to change notification settings - Fork 23
Add protovalidate as alternative to protoc-gen-validate #124
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
Add protovalidate as alternative to protoc-gen-validate #124
Conversation
Migrate files and re-add old validate temporarily
5199e7b to
0f29d31
Compare
bc7dbea to
6676de4
Compare
|
The latest Buf updates on your PR. Results from workflow Lint / lint (pull_request).
|
josephschorr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.github/workflows/release.yaml
Outdated
| - uses: "actions/checkout@v3" | ||
| - uses: "bufbuild/buf-setup-action@v1.32.2" | ||
| - uses: "actions/checkout@v4" | ||
| - uses: "bufbuild/buf-setup-action@v1.47.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buf-setup-action was deprecated in favor of buf-action: https://github.com/bufbuild/buf-setup-action#buf-setup-action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm using it in the lint action. I can update this usage as well.
| @@ -1,15 +1,21 @@ | |||
| --- | |||
| version: "v1" | |||
| version: "v2" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, did you use buf config migrate? https://buf.build/docs/migration-guides/migrate-v2-config-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
tstirrat15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committing
| import "authzed/api/v1/core.proto"; | ||
|
|
||
| option go_package = "github.com/authzed/authzed-go/proto/authzed/api/materialize/v0"; | ||
| option java_package = "com.authzed.api.materialize.v0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes to imports are a part of running buf format
.github/workflows/lint.yaml
Outdated
| - uses: "bufbuild/buf-setup-action@v1.47.2" | ||
| with: | ||
| version: "1.30.0" | ||
| github_token: "${{ github.token }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This authenticates our buf requests
| repos: | ||
| - repo: "https://github.com/bufbuild/buf" | ||
| rev: "v1.6.0" | ||
| rev: "v1.47.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that we're linting with a recent version of buf
| @@ -1,15 +1,21 @@ | |||
| --- | |||
| version: "v1" | |||
| version: "v2" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
.github/workflows/release.yaml
Outdated
| - uses: "actions/checkout@v3" | ||
| - uses: "bufbuild/buf-setup-action@v1.32.2" | ||
| - uses: "actions/checkout@v4" | ||
| - uses: "bufbuild/buf-setup-action@v1.47.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm using it in the lint action. I can update this usage as well.
7ac2968
6676de4 to
7ac2968
Compare
Description
I ran into this while looking into
authzed-rb's implementation of validation. I hoped that it might solve that problem, but it turns out that there isn't aprotovalidateimplementation for ruby either (yet, I hope).I think there's still value in bringing this in, though.
protoc-gen-validateis in maintenance mode: https://github.com/bufbuild/protoc-gen-validate?tab=readme-ov-file#-protoc-gen-validate-pgvThe library they want us to move to is
protovalidate: https://github.com/bufbuild/protovalidateThis follows the steps in the migration guide to add annotations that are compatible with
protovalidateand allow the downstream libs to use theprotovalidatelibs as desired.Notes
The breaking change lint step is complaining, but I ran it locally and it's not code that we use directly or control:
It appears to be complaining about the proto provided by the
grpc-gatewaydep, and that version bump happened when I added the new dependency. I'm not particularly concerned about it breaking anything.Changes
buf.yamlto v2buf formatover everythingNotes
This does not remove support for protoc-gen-validate. We probably won't do that for a while yet. This should be a non-breaking change.
Testing
Review. See that our CI still passes.