Skip to content

Conversation

@smaye81
Copy link
Contributor

@smaye81 smaye81 commented Apr 18, 2025

The unit tests in this repo use Protobuf definitions from the conformance suite, which works but it makes debugging issues a bit difficult. It is extremely helpful to have local protos in the repo that can be changed for tests to debug issues.

This follows the example in protovalidate-go by creating a validations.proto and copies the definitions currently being used from conformance into this proto.

Comment on lines +36 to +38
$(BIN)/buf generate buf.build/bufbuild/protovalidate:$(PROTOVALIDATE_VERSION)
$(BIN)/buf generate buf.build/bufbuild/protovalidate-testing:$(PROTOVALIDATE_VERSION)
$(BIN)/buf generate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's helpful to have local proto files for debugging. But PROTOVALIDATE_VERSION and buf.lock competing over the exact version seems like a footgun.

Maybe the least controversial thing to do is to cut a protovalidate release that contains the new test protos?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not following. pv-go is doing this exact thing with no issues. I would really prefer for this to follow that pattern. Being able to change protos while debugging issues is much quicker than constantly having to cut protovalidate releases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good that we haven't had an issue with it so far. I didn't realize the Go implementation already does it. LGTM, and hopefully find a better solution down the road.

@smaye81 smaye81 merged commit 982f604 into main Apr 21, 2025
13 checks passed
@smaye81 smaye81 deleted the sayers/add_local_protos branch April 21, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants