rvolosatovs
commented
4 years ago The forks are required, because:
protoc-gen-star: We need these merged https://github.com/lyft/protoc-gen-star/pull/67 https://github.com/lyft/protoc-gen-star/pull/51, once they are merged the fork can be removedprotoc-gen-validate: Upstream generates validators per-message, while we want to have them per-field, modifying the expectedValidate() errorsignature to allow fields to be specified is breaking and adding an additional assertion to check forValidateFields(...string) error, like we expect would add additional overhead, which I don't think they would be happy about. In our case we also do not want to have such overhead. That said, I suppose a possibility would be to make it configurable and generate per-message validators by default and generate per-field validators(or both) if configured to do so. At this point I don't think we can justify spending time and resources on doing this, given that we do not gain anything from it - the package is meant to be used as a binary anyway and compiles perfectly fine with no magic https://github.com/TheThingsIndustries/docker-protobuf/blob/7220a24abbcdcc54dd965e5f83aea16f676468d7/Dockerfile#L66-L67 If you're willing to contribute to this - please, by all means, feel free to do so and we will be happy to accept it. FTR, here's the diff we're using currently https://github.com/envoyproxy/protoc-gen-validate/compare/v0.3.0-java...TheThingsIndustries:v0.3.0-java-fieldmask.3
codyaray
There are 2 forked dependencies in use: https://github.com/TheThingsIndustries/protoc-gen-validate and https://github.com/TheThingsIndustries/protoc-gen-star
This causes troubles becase all consumers of
protoc-gen-fieldmaskmodule also have to addreplacedirectives to their go.mod files, e.g.Is there any strong reason to keep using these forks? Are this forks long-running i.e. the updates will never land into upstream? If so, maybe we should rework them so only the required functionality is kept, and other is discarded, to break upstream dependency.
As another option, would it make sense to move required functionality from https://github.com/TheThingsIndustries/protoc-gen-validate directly to this repository and again remove the dependency? E.g. as
protoc-gen-fieldmasksupports only Go we don't need upstream features for Java and Python. Probably we could just copy required code here so it's all in one place.WDYT?
Sorry if my questions make no sense, as I may have incorrect assumptions about these forks.