You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/06/03 08:36:33 UTC
[GitHub] [apisix] yuhongyu879 opened a new pull request #4368: upgrade protobuf to 0.3.2
yuhongyu879 opened a new pull request #4368:
URL: https://github.com/apache/apisix/pull/4368
protobufjs use custom wrapper for google.protobuf.any and other types, if we want decode message like this, we can use pb.hook which introduced in 0.3.2, but after upgrade lua-protobuf to that version, the [grpc-transcode plugin](https://github.com/apache/apisix/blob/62ceab5710859d39828c6a88e3602ab6df1a589f/apisix/plugins/grpc-transcode/proto.lua#L48) won't work because of this [mr](https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e), so as a quick fix, we can pass a filename to load function, or if needed we can write a function does the same as the lib function do but return the resolve message info additionally.
thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 commented on pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 commented on pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#issuecomment-854455158
@spacewander Whom could i ask for approving? It says that 'Only reviews by reviewers with write access count toward mergeability' on @Firstsawyou 's approving
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers merged pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
tokers merged pull request #4368:
URL: https://github.com/apache/apisix/pull/4368
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r644673459
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,7 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ local res = _p:load(content, "filename for loaded") -- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
Review comment:
```suggestion
-- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
local res = _p:load(content, "filename for loaded")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r645230876
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
```suggestion
-- after _p:load(content) is always empty, so we can pass a fake file name to keep the code below unchanged, or we can create
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r644757140
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,7 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ local res = _p:load(content, "filename for loaded") -- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
Review comment:
how about the explanation now?
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
thx
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
thx
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
oh should i use ur suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r645256710
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r644757140
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,7 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ local res = _p:load(content, "filename for loaded") -- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
Review comment:
how about the explanation now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#issuecomment-854302865
Please fix the lint
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r644709102
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,7 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ local res = _p:load(content, "filename for loaded") -- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
Review comment:
We need to explain it clearly in the comment instead of just a commit link.
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
```suggestion
-- after _p:load(content) is always empty, so we can pass a fake file name to keep the code below unchanged, or we can create
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r644673459
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,7 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ local res = _p:load(content, "filename for loaded") -- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
Review comment:
```suggestion
-- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
local res = _p:load(content, "filename for loaded")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 edited a comment on pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 edited a comment on pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#issuecomment-854455158
@spacewander Whom could i ask for another approving? It says that 'Only reviews by reviewers with write access count toward mergeability' on @Firstsawyou 's approving
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r645258937
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
oh should i use ur suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r644709102
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,7 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ local res = _p:load(content, "filename for loaded") -- https://github.com/starwing/lua-protobuf/commit/89faac205585360d800472c62752d5b532d7ce9e
Review comment:
We need to explain it clearly in the comment instead of just a commit link.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yuhongyu879 commented on a change in pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
yuhongyu879 commented on a change in pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#discussion_r645258616
##########
File path: apisix/plugins/grpc-transcode/proto.lua
##########
@@ -43,7 +43,11 @@ local function create_proto_obj(proto_id)
end
local _p = protoc.new()
- local res = _p:load(content)
+ -- the loaded proto won't appears in _p.loaded without a file name after lua-protobuf=0.3.2, which means _p.loaded
+ -- after _p:load(content) is always empty, so we can pass a default to keep the codes below unchanged, or we can create
Review comment:
thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4368: feat: upgrade protobuf to 0.3.2
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4368:
URL: https://github.com/apache/apisix/pull/4368#issuecomment-854302865
Please fix the lint
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org