Update CI #24
Update CI #24
Conversation
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
| @@ -25,141 +25,141 @@ import ( | |||
| v1 "github.com/apache/skywalking-banyandb/api/proto/banyandb/v1" | |||
| ) | |||
|
|
|||
| type shardEventBuilder struct { | |||
| type ShardEventBuilder struct { | |||
There was a problem hiding this comment.
The exported func in this file return unexported types, which causes the lint check to fail. @lujiajing1126 Would you pls check whether the way I fixed is correct?
There was a problem hiding this comment.
The exported func in this file return unexported types, which causes the lint check to fail. @lujiajing1126 Would you pls check whether the way I fixed is correct?
Yes. I suppose the change is correct. I intended not to expose/export the structs and only allow users to create builder(s) by the given functions.
I don't know why this pattern is not recommended by the linter. Ref: https://stackoverflow.com/a/21470517
There was a problem hiding this comment.
It's not a common pattern in go. If you want to force users to use the factory method, an interface should be better, which is a canonical pattern to gophers.
There was a problem hiding this comment.
It's not a common pattern in go. If you want to force users to use the factory method, an interface should be better, which is a canonical pattern to gophers.
Sure. Make sense to me.
There was a problem hiding this comment.
It's not a common pattern in go. If you want to force users to use the factory method, an interface should be better, which is a canonical pattern to gophers.
I did some investigation. It seems this pattern does commonly used by developers.
See golang/lint#210
| use: | ||
| - DEFAULT | ||
| except: | ||
| - RPC_REQUEST_STANDARD_NAME |
There was a problem hiding this comment.
As I asked in the last PR #23 , do we need to impose these two rules?
There was a problem hiding this comment.
It's the standard/common pattern to name the request. skywalking keeps using this pattern to build grpc service.
There was a problem hiding this comment.
It's the standard/common pattern to name the request. skywalking keeps using this pattern to build grpc service.
So if we need to comply with the conventions, we have to change the request and response types as well as delete this except section.
There was a problem hiding this comment.
Would you fix this issue?
There was a problem hiding this comment.
Would you fix this issue?
Sure I can do this later.
| @rm -rf $(tool_include)/google | ||
| @mkdir -p $(tool_bin) $(tool_include) | ||
| $(eval protoc_tmp := $(shell mktemp -d)) | ||
| cd $(protoc_tmp); curl -sSL https://github.com/protocolbuffers/protobuf/releases/download/v$(protoc_version)/protoc-$(protoc_version)-$(protoc_os)-$(protoc_arch).zip -o protoc.zip |
There was a problem hiding this comment.
Just a comment: this cannot work on osx-arm64 platform, i.e. Apple Chip.
(Unfortunately, I changed my working laptop last week....🤣)
There was a problem hiding this comment.
you can tweak it to build protoc from sources, or copy protoc into bin directory in the root path.
|
LGTM |
Signed-off-by: Gao Hongtao hanahmily@gmail.com