-
Notifications
You must be signed in to change notification settings - Fork 292
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 lint rules #2473
Merged
Merged
Changes from 29 commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
40e989e
basic
oliversun9 8bf543b
comment
oliversun9 2afea78
make protosource.File expose a protodescriptor.FileDescriptor
oliversun9 9de109d
fix type loading for field
oliversun9 0bfcf63
remove remote package, generate validate.pb.go locally
oliversun9 9833c7c
name
oliversun9 3698f50
pointer
oliversun9 aa2ef7d
style
oliversun9 c1aee25
test
oliversun9 abc0e8d
field name
oliversun9 5119bce
test
oliversun9 3dc3dbf
use celext
oliversun9 69f9cba
lint
oliversun9 f84b653
Merge branch 'main' into osun/buf-lint-cel
oliversun9 3d9d2e5
comment
oliversun9 242a97d
doc
oliversun9 662bbbc
use protovalidate-go v0.3.3
oliversun9 5e33b02
use DefaultResolver from protovalidate-go, which in turn requires mov…
oliversun9 7188999
small refactor
oliversun9 59e3caf
remove dynamic extension types as they are no longer needed
oliversun9 3437aaa
rename
oliversun9 1215364
commit
bufdev c1016d2
commit
bufdev 54d2489
commit
bufdev c15c65a
Merge branch 'main' into osun/buf-lint-cel
bufdev e842c9a
commit
bufdev 3b32cde
add test for parsing error texts from cel-go; comments
oliversun9 cc10191
lint
oliversun9 13c2ebd
Merge branch 'main' into osun/buf-lint-cel
oliversun9 30a07fc
rename sub-directory for testcase
oliversun9 05e46db
use new dir in test
oliversun9 0ce125c
dont allow cel expressions on fields of type google.protobuf.Any
oliversun9 c9bef9d
fix make upgrade
bufdev a66aaa7
merge
bufdev a33ac0d
Add support for linting protovalidate rules (#2505)
oliversun9 8b474dc
fix comment
oliversun9 da8f4b0
merge main
oliversun9 742558f
bump protovalidate to v0.5.1
oliversun9 8aa53b8
update links
oliversun9 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
40 changes: 40 additions & 0 deletions
40
private/bufpkg/bufcheck/buflint/internal/buflintvalidate/buflintvalidate.go
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| // Copyright 2020-2023 Buf Technologies, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package buflintvalidate | ||
|
|
||
| import ( | ||
| "github.com/bufbuild/buf/private/pkg/protosource" | ||
| "google.golang.org/protobuf/reflect/protodesc" | ||
| ) | ||
|
|
||
| // ValidateCELCompiles validates that all CEL expressions defined for protovalidate | ||
| // in the given file compile. | ||
| func ValidateCELCompiles( | ||
| resolver protodesc.Resolver, | ||
| add func(protosource.Descriptor, protosource.Location, []protosource.Location, string, ...interface{}), | ||
| file protosource.File, | ||
| ) error { | ||
| for _, message := range file.Messages() { | ||
| if err := validateCELCompilesMessage(resolver, add, message); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, extensionField := range file.Extensions() { | ||
| if err := validateCELCompilesField(resolver, add, extensionField); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
Oops, something went wrong.
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.
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.
Is this true? What bootstrapping errors do we expect? If we're using the
bufCLI to work onbuf, we're working with a binary where the remote packages are already resolved and compiled, so I don't see how this is a problem here.I was under the impression that it's the core repo that must not depend on generated SDKs. Note that this directive here will do nothing to prevent a cyclic dependency in core since replace directives in dependencies are ignored by the go tool.
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.
The
bufCLI's binary itself will be fine, since like you said, it's just a binary.Yes I think this is the concern and we will also need to add a replace directive there, before or around the time when this PR is merged.
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 we can remove the replace directive here?
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.
Great question, and with the reasoning above I think it's probably fine? @bufdev what do you think?