Skip to content
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

fix descriptor path for golang #443

Merged
merged 4 commits into from
Apr 7, 2021
Merged

fix descriptor path for golang #443

merged 4 commits into from
Apr 7, 2021

Conversation

danielhochman
Copy link
Contributor

@danielhochman danielhochman commented Mar 17, 2021

Although the import path is validate/validate.proto the registered path for the descriptor changed to validate.proto in 0.5.0. This breaks gRPC reflection in Go with the error no such file: "validate/validate.proto".

Fixes #442

Signed-off-by: Daniel Hochman <danielhochman@users.noreply.github.com>
Signed-off-by: Daniel Hochman <danielhochman@users.noreply.github.com>
@danielhochman
Copy link
Contributor Author

Unfortunately there a lot of additional updates in the generated file because the version of protoc-gen-go specified in the Dockerfile (I used the Dockerfile to generate) is ultimately overriden by go.mod. Also from what I can tell CI doesn't check whether or not the file was correctly updated on each PR so things can drift quite a bit without anyone catching it.

@akonradi
Copy link
Contributor

Thanks for the fix! Sounds like we should add a rule to CI to check that the checked-in generated code is in sync. Is that something you could tackle in this PR?

@danielhochman
Copy link
Contributor Author

@akonradi i filed #444 for that with some examples of how we do it in lyft/clutch.

@akonradi
Copy link
Contributor

Thanks. My hope was that you'd do it here since the only way I have to review this PR as-is is to check it out locally, regenerate the code myself, and check for diffs. If that sounds suspiciously like what lyft/clutch does for CI, it's because it is 😄 If the CI change is not feasible to do in this PR I'm happy to review manually.

@danielhochman
Copy link
Contributor Author

@akonradi The diff check itself is simple. I'm not familiar with the build process in the repo between Bazel, Docker, and Make, to know exactly where to include the generation and diff it. From poking around, I'm not even sure the code is generated at all in CI at the moment. And builds are 30+ minutes so iteration time would be quite slow unfortunately.

check it out locally, regenerate the code myself, and check for diffs. If that sounds suspiciously like what lyft/clutch does for CI, it's because it is 😄

I didn't exactly follow this, Clutch automatically checks the diff on every PR in CI using GitHub Actions. I think that's what you meant but it read like the opposite to me.

@akonradi
Copy link
Contributor

Alright so I've checked out your PR and regenerated validate.pb.go using Bazel. It looks like it's using a different version of protoc than make, which is funny, and related to #435.

@danielhochman
Copy link
Contributor Author

Yeah I noticed that as well.

Alternatively, for this PR I could go back to the last commit where the file was changed and regen with my Makefile change to produce what is hopefully a really small diff. Then we could follow up with separate PRs to regen with new deps, trying to make versions match between Bazel and make, and finally try to land other CI fixes to reduce duplication, verify regeneration, etc.

akonradi added a commit to akonradi/protoc-gen-validate that referenced this pull request Mar 18, 2021
This currently only covers validate/validate.pb.go but should be
sufficient to catch regressions like the one fixed in bufbuild#443.

Towards bufbuild#444

Signed-off-by: Alex Konradi <akonradi@google.com>
@akonradi
Copy link
Contributor

I'm not familiar with the build process in the repo

Not a problem. I've pushed akonradi@fd45adc in my fork that is a simplified version of what lyft/clutch does. If you want to pull that in here, I think that will both prove that the checked-in version of validate.pb.go is correct and prevent this from happening again.

danielhochman and others added 2 commits April 7, 2021 12:15
This currently only covers validate/validate.pb.go but should be
sufficient to catch regressions like the one fixed in #443.

Towards #444

Signed-off-by: Alex Konradi <akonradi@google.com>
@danielhochman
Copy link
Contributor Author

@akonradi mind taking a look? i cherry picked your validation commit

@akonradi
Copy link
Contributor

akonradi commented Apr 7, 2021

Yep, was just waiting for CI to pass. Thanks!

@akonradi akonradi merged commit 9f03680 into bufbuild:main Apr 7, 2021
@danielhochman danielhochman deleted the fix-descriptor-registration-go branch April 7, 2021 18:18
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.

golang descriptor registers to incorrect path
2 participants