bufbuild / buf

The best way of working with Protocol Buffers.
https://buf.build
Apache License 2.0
8.49k stars 247 forks source link

buf 1.32.2 breaking doesn't respect v2 config file when using binpb input #3080

Closed dnr closed 6 days ago

dnr commented 2 weeks ago

What's up?

Sorry for lack of test case, but this should be pretty easy to reproduce:

I was trying to set up a check using buf breaking like this:

buf breaking image1.bin --against image2.bin --config buf.yaml

The image files were not generated by buf, they were generated by protoc -o

I tried using buf 1.32.2 with a config like:

version: v2
breaking:
  use:
    - WIRE

This did the breaking check using the default FILE level, the WIRE level was not respected. I also tried adding ignore settings that were not respected.

I downgraded to buf 1.6.0 and used the same command line with the same config except version: v1, and it respected the breaking settings as expected.

doriable commented 2 weeks ago

I am unable to reproduce the error state. I built an example that fails FILE level checks but passes WIRE level checks, built images with protoc -o, and did not get any check failures with:

buf breaking image1.bin --against image2.bin --config buf.yaml

Could you please provide a repro of the problems you're running into?

bufdev commented 2 weeks ago

Closing issue until reproduction is provided - we ask for repros as our team gets a lot of issues, and needs to be able to efficiently investigate them. If you can provide a reproduction, we will be happy to re-open!

dnr commented 2 weeks ago

Fair enough, and thanks for trying!

I reduced my case to a minimal repro using buf 1.32.2 and just a v1 vs v2 config file, with binary images with a single renamed field:

https://github.com/dnr/buf-repro

$ ./run.sh
Buf version:
1.32.2

Correctly uses 'WIRE' rules (no output):

Uses rules besides 'WIRE':
temporal/server/api/clock/v1/message.proto:1:1:Field "1" with name "wll_clock" on message "HybridLogicalClock" changed option "json_name" from "wallClock" to "wllClock".
temporal/server/api/clock/v1/message.proto:1:1:Field "1" on message "HybridLogicalClock" changed name from "wall_clock" to "wll_clock".

(edit: oops, repo was marked private initially, changed to public now)

dnr commented 1 week ago

@doriable could you take another look, or let me know if I missed something obvious with the repro?

doriable commented 6 days ago

This issue was resolved in the PR :) It will go out with the next release!