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

Replace prototool with buf #847

Merged
merged 7 commits into from
Mar 17, 2021
Merged

Conversation

johanbrandhorst
Copy link
Contributor

Changes

Replace prototool generation and linting with buf

Verification

Reran generation and lint locally. Need to see what CI thinks about the changes.

@improbable-prow-robot improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 600+ lines, ignoring generated files. labels Feb 3, 2021
@johanbrandhorst johanbrandhorst force-pushed the replace-prototool-with-buf branch from 3db5016 to c74f699 Compare February 3, 2021 10:53
Comment on lines +9 to +22
ENUM_VALUE_PREFIX:
- improbable/grpcweb/test/test.proto
ENUM_ZERO_VALUE_SUFFIX:
- improbable/grpcweb/test/test.proto
FIELD_LOWER_SNAKE_CASE:
- improbable/grpcweb/test/test.proto
PACKAGE_VERSION_SUFFIX:
- improbable/grpcweb/test/test.proto
RPC_REQUEST_RESPONSE_UNIQUE:
- improbable/grpcweb/test/test.proto
RPC_REQUEST_STANDARD_NAME:
- improbable/grpcweb/test/test.proto
RPC_RESPONSE_STANDARD_NAME:
- improbable/grpcweb/test/test.proto
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ignores are based on the existing failures in this file. These are the warnings:

$ buf lint
integration_test/proto/improbable/grpcweb/test/test.proto:3:1:Package name "improbable.grpcweb.test" should be suffixed with a correctly formed version, such as "improbable.grpcweb.test.v1".
integration_test/proto/improbable/grpcweb/test/test.proto:10:5:Enum value name "NONE" should be prefixed with "FAILURE_TYPE_".
integration_test/proto/improbable/grpcweb/test/test.proto:10:5:Enum zero value name "NONE" should be suffixed with "_UNSPECIFIED".
integration_test/proto/improbable/grpcweb/test/test.proto:11:5:Enum value name "CODE" should be prefixed with "FAILURE_TYPE_".
integration_test/proto/improbable/grpcweb/test/test.proto:13:5:Enum value name "CODE_UNICODE" should be prefixed with "FAILURE_TYPE_".
integration_test/proto/improbable/grpcweb/test/test.proto:28:10:Field name "Value" should be lower_snake_case, such as "value".
integration_test/proto/improbable/grpcweb/test/test.proto:39:3:".google.protobuf.Empty" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:39:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:39:17:RPC request type "Empty" should be named "PingEmptyRequest" or "TestServicePingEmptyRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:39:49:RPC response type "PingResponse" should be named "PingEmptyResponse" or "TestServicePingEmptyResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:41:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:41:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:43:3:".google.protobuf.Empty" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:43:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:43:17:RPC request type "PingRequest" should be named "PingErrorRequest" or "TestServicePingErrorRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:43:39:RPC response type "Empty" should be named "PingErrorResponse" or "TestServicePingErrorResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:45:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:45:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:45:16:RPC request type "PingRequest" should be named "PingListRequest" or "TestServicePingListRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:45:45:RPC response type "PingResponse" should be named "PingListResponse" or "TestServicePingListResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:47:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:47:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:47:27:RPC request type "PingRequest" should be named "PingPongBidiRequest" or "TestServicePingPongBidiRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:47:56:RPC response type "PingResponse" should be named "PingPongBidiResponse" or "TestServicePingPongBidiResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:49:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:49:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:49:25:RPC request type "PingRequest" should be named "PingStreamRequest" or "TestServicePingStreamRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:49:47:RPC response type "PingResponse" should be named "PingStreamResponse" or "TestServicePingStreamResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:51:3:RPC "Echo" has the same type ".improbable.grpcweb.test.TextMessage" for the request and response.
integration_test/proto/improbable/grpcweb/test/test.proto:51:12:RPC request type "TextMessage" should be named "EchoRequest" or "TestServiceEchoRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:51:34:RPC response type "TextMessage" should be named "EchoResponse" or "TestServiceEchoResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:67:3:".google.protobuf.Empty" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:67:54:RPC response type "Empty" should be named "ContinueStreamResponse" or "TestUtilServiceContinueStreamResponse".
integration_test/proto/improbable/grpcweb/test/test.proto:73:3:".improbable.grpcweb.test.PingRequest" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:73:3:".improbable.grpcweb.test.PingResponse" is used as the request or response type for multiple RPCs.
integration_test/proto/improbable/grpcweb/test/test.proto:73:19:RPC request type "PingRequest" should be named "NonExistantRequest" or "FailServiceNonExistantRequest".
integration_test/proto/improbable/grpcweb/test/test.proto:73:41:RPC response type "PingResponse" should be named "NonExistantResponse" or "FailServiceNonExistantResponse".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to fix any warnings in this PR to make the impact as small as possible.

fi

curl -sSL \
https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VER}/protoc-${PROTOC_VER}-linux-$(uname -m).zip \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this doesn't use uname -s since protoc uses lowercase names. I figured since this is for CI it doesn't matter, but I can make probably do a lowercase transformation somehow if necessary.

@johanbrandhorst
Copy link
Contributor Author

Tests that failed are giving me an npm error:

/tmp/.bash_env-601a8141e5331f7178d9a11c-0-build: line 2: /home/circleci/.nvm/nvm.sh: No such file or directory
/bin/bash: npm: command not found

No idea what's going on there, any ideas @MarcusLongmuir?

@johanbrandhorst
Copy link
Contributor Author

Rerunning the tests now.

@MarcusLongmuir
Copy link
Contributor

  1. Do we still need to install these?
- run: go install ./vendor/github.com/golang/protobuf/protoc-gen-go
- run: export PATH=/go/src/github.com/improbable-eng/grpc-web/protobuf/bin:$PATH
  1. What's the relationship between buf and protoc in this configuration? From my understanding buf doesn't require protoc, but it's optional.

@johanbrandhorst
Copy link
Contributor Author

Do we still need to install these?

  • run: go install ./vendor/github.com/golang/protobuf/protoc-gen-go
  • run: export PATH=/go/src/github.com/improbable-eng/grpc-web/protobuf/bin:$PATH

We still have to install the protobuf plugins, as buf doesn't handle language specific generation. This is consistent with protoc when not looking at the built-in generators.

What's the relationship between buf and protoc in this configuration? From my understanding buf doesn't require protoc, but it's optional.

Indeed, buf doesn't depend on protoc, however, since we're generating using --js_out we need the JavaScript generator plugin, which is built into protoc, so that's the only reason we still have to download protoc.

@johanbrandhorst johanbrandhorst marked this pull request as ready for review February 5, 2021 12:24
@improbable-prow-robot improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2021
@johanbrandhorst
Copy link
Contributor Author

Bump

@johanbrandhorst
Copy link
Contributor Author

Rebased on master and removed some old dep references I missed in the modules migration, please take a look when you can @MarcusLongmuir.

Copy link
Contributor

@MarcusLongmuir MarcusLongmuir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this 👍

.circleci/config.yml Show resolved Hide resolved
integration_test/package.json Show resolved Hide resolved
@johanbrandhorst johanbrandhorst force-pushed the replace-prototool-with-buf branch from bd79d71 to b89cb7f Compare March 17, 2021 09:50
@johanbrandhorst
Copy link
Contributor Author

Hm, not sure about the test errors, could it be flakes? The logic shouldn't have changed and all the other tests passing is suspicious.

@MarcusLongmuir
Copy link
Contributor

Looks like the testserver is panicing with panic: rpc error: code = Unavailable desc = transport is closing. This is unrelated so it's safe to merge this.

@MarcusLongmuir
Copy link
Contributor

You should be able to retry just the failing tests and merge when they pass, but comment if that doesn't work.

@johanbrandhorst johanbrandhorst merged commit 84ab65f into master Mar 17, 2021
@johanbrandhorst johanbrandhorst deleted the replace-prototool-with-buf branch March 17, 2021 10:39
@johanbrandhorst
Copy link
Contributor Author

Thanks for the quick review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 600+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants