search

namely / docker-protoc

Docker images for generating protocol buffer definitions
BSD 3-Clause "New" or "Revised" License
726 stars 225 forks source link

bump protoc-gen-validate to latest version to support validation on proto3 optional #297

Closed zaquestion closed 2 years ago

zaquestion commented 2 years ago

Installed protoc-gen-go before protoc-gen-validate as that's a dependency now. Tho I left the later install just incase the version gets overwritten along the way (inline with #210). I believe the existing tests cover us well enough here. I did go ahead and build all this locally and test generation with protoc-gen-validate and saw the expected results as well. Would deeply appreciate bringing this in as it's blocking a number of other things at my work.

zaquestion commented 2 years ago

Hi @ido-namely need some guidance on how best to move this forward. I added to the all/test/test.proto an optional field and executed the tests on it. It causes the protoc-gen-grpc-gateway plugin to fail as it's pined to a fairly old version (10 minors behind). Updating that causes some fairly specific tests to break as the generation for some FieldMask seems to change in later versions. Also even if it didn't this PR gains a much larger scope if it's updating multiple deps.

I believe I can split out into a separate test_validate.proto file and write a test against that and not update any other deps, just validating the behavior changes of this PR. Would that be suitable for merge, or would you like some other route?

ido-namely commented 2 years ago

Hi @ido-namely need some guidance on how best to move this forward. I added to the all/test/test.proto an optional field and executed the tests on it. It causes the protoc-gen-grpc-gateway plugin to fail as it's pined to a fairly old version (10 minors behind). Updating that causes some fairly specific tests to break as the generation for some FieldMask seems to change in later versions. Also even if it didn't this PR gains a much larger scope if it's updating multiple deps.

I believe I can split out into a separate test_validate.proto file and write a test against that and not update any other deps, just validating the behavior changes of this PR. Would that be suitable for merge, or would you like some other route?

Hi @zaquestion , creating a separate file sounds like a good solution at this point. Thank you!

ido-namely commented 2 years ago

Hi @zaquestion , Are you still planning to work on this PR?

Thank you.

ido-namely commented 2 years ago

replaced by https://github.com/namely/docker-protoc/pull/311