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

Make support paths option #711

Merged
merged 1 commit into from
Aug 2, 2018
Merged

Conversation

izumin5210
Copy link
Contributor

Make protoc-gen-grpc-gateway support paths option such like golang/protobuf#515

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@izumin5210
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@johanbrandhorst
Copy link
Collaborator

Hi @izumin5210, this looks like a great addition! I'm a little bit confused by the build error, looks like the files need to be regenerated, but I don't think your change as anything to do with it. Could you update the commit message to contain the same kind of description that your pull request does please?

@izumin5210
Copy link
Contributor Author

@johanbrandhorst I fixed the message! 😄

@johanbrandhorst
Copy link
Collaborator

@achew22 @tmc what's going on with CI here?

@achew22
Copy link
Collaborator

achew22 commented Jul 30, 2018

@johanbrandhorst Very strange. We used to have this kind of problem all the time. It was from proto-go being upgraded and changing the output format. When there are no env variables set, we compare the compiled versions of the .pb.go to the version that is checked in for equality (golden testing).

However, with the submission of #696, I wouldn't expect that to happen any more. I don't know a lot about dep unfortunately. I was hoping we could just wait till vgo, but I'm glad we did this. Looking at this line is it possible that we are overriding our go dep and explicitly installing the HEAD version of proto-go?

The other two failures appear to just be standard tooling errors. If we paid for Travis I bet it would be more stable, but the free version is kind of flaky. I re-triggered them and they passed.

@johanbrandhorst
Copy link
Collaborator

That line is only installing golint, it shouldn't affect the version of protoc-gen-go.

@achew22
Copy link
Collaborator

achew22 commented Jul 31, 2018

@johanbrandhorst, I wasn't meaning to imply that line specifically, but is there something in that file you would expect is updating our dependencies to head instead of the vendored versions?

@johanbrandhorst
Copy link
Collaborator

No, I cant see how it would do that. I agree I figured itd be fixed by the dependency locking. I guess we'll have to do some more digging.

@achew22
Copy link
Collaborator

achew22 commented Aug 2, 2018

I fixed it by manually applying the patch. Could you please rebase this PR to see how Travis feels about it now? Thanks!

Make protoc-gen-grpc-gateway support paths option such like golang/protobuf#515.
@izumin5210
Copy link
Contributor Author

I rebased!

But CI failed again 😢
Could you rerun them?

15.27s$ test "${USE_BAZEL}" = true || go get github.com/golang/lint/golint
package golang.org/x/lint: unrecognized import path "golang.org/x/lint" (https fetch: Get https://golang.org/x/lint?go-get=1: net/http: TLS handshake timeout)
The command "test "${USE_BAZEL}" = true || go get github.com/golang/lint/golint" failed and exited with 1 during .

https://travis-ci.org/grpc-ecosystem/grpc-gateway/jobs/411080943#L557-L561

@johanbrandhorst
Copy link
Collaborator

Looks like it's all working again, thanks for your contribution!

@johanbrandhorst johanbrandhorst merged commit 42fa202 into grpc-ecosystem:master Aug 2, 2018
@izumin5210 izumin5210 deleted the paths branch August 2, 2018 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants