You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/06/03 05:56:10 UTC
[GitHub] [skywalking-satellite] EvanLjp opened a new pull request #43: change go api package
EvanLjp opened a new pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43
remove old genernate operation in CI & change package
--
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] [skywalking-satellite] kezhenxu94 commented on a change in pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#discussion_r644518004
##########
File path: protocol/satellite-protocol/satellite/Event.proto
##########
@@ -20,8 +20,7 @@ syntax = "proto3";
package skywalking.v3;
-option go_package = "satellite/protocol";
-
+option go_package = "skywalking.apache.org/repo/goapi/satellite/event/v1";
Review comment:
How does it work to just rename the package without recompile the proto files into go files?
--
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] [skywalking-satellite] EvanLjp commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
EvanLjp commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853687514
okay
--
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] [skywalking-satellite] kezhenxu94 commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853658558
--
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] [skywalking-satellite] kezhenxu94 commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853658558
> Consider adding `satellite/internal` folder into goapi repo, and move proto with generated source codes there. Is this better?
import paths containing `internal` is not available outside of the `goapi` module, this is restricted by Go compiler if I remember correctly
--
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] [skywalking-satellite] kezhenxu94 commented on a change in pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#discussion_r644517661
##########
File path: Makefile
##########
@@ -54,15 +54,6 @@ tools:
deps: tools
$(GO_GET) -v -t -d ./...
-.PHONY: gen-codes
-gen-codes:
- $(PROTOC) --version || sh tools/install_protoc.sh
- /bin/sh tools/protocol_gen.sh
-
-.PHONY: gen-docs
-gen-docs: build
- rm -rf $(PLUGIN_DOC_DIR)
- $(OUT_DIR)/$(BINARY)-$(VERSION)-$(OSNAME)-$(ARCH) docs --output=$(PLUGIN_DOC_DIR)
Review comment:
I'm not sure why you remove these 2 targets, if you do, please also remove the related files, such as `tools/protocol_gen.sh`, as it's not used and it doesn't work now since you modify the package name.
##########
File path: protocol/satellite-protocol/satellite/Event.proto
##########
@@ -20,8 +20,7 @@ syntax = "proto3";
package skywalking.v3;
-option go_package = "satellite/protocol";
-
+option go_package = "skywalking.apache.org/repo/goapi/satellite/event/v1";
Review comment:
How does it work to just rename the package without recompile the proto files into go files?
--
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] [skywalking-satellite] wu-sheng merged pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43
--
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] [skywalking-satellite] kezhenxu94 commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853664761
Now the import path in this repo is `github.com/apache/skywalking-satellite/protocol/gen-codes/satellite/protocol`, I think adding `satellite` folder in `goapi` repo is enough, 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] [skywalking-satellite] wu-sheng commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853660894
> > Consider adding `satellite/internal` folder into goapi repo, and move proto with generated source codes there. Is this better?
>
> import paths containing `internal` is not available outside of the `goapi` module, this is restricted by Go compiler if I remember correctly
Any suggestion? My point is only hosting generated codes in `goapi` repo.
--
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] [skywalking-satellite] wu-sheng commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853676026
@EvanLjp Take a look.
--
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] [skywalking-satellite] wu-sheng commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853642767
--
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] [skywalking-satellite] kezhenxu94 commented on a change in pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#discussion_r644517661
##########
File path: Makefile
##########
@@ -54,15 +54,6 @@ tools:
deps: tools
$(GO_GET) -v -t -d ./...
-.PHONY: gen-codes
-gen-codes:
- $(PROTOC) --version || sh tools/install_protoc.sh
- /bin/sh tools/protocol_gen.sh
-
-.PHONY: gen-docs
-gen-docs: build
- rm -rf $(PLUGIN_DOC_DIR)
- $(OUT_DIR)/$(BINARY)-$(VERSION)-$(OSNAME)-$(ARCH) docs --output=$(PLUGIN_DOC_DIR)
Review comment:
I'm not sure why you remove these 2 targets, if you do, please also remove the related files, such as `tools/protocol_gen.sh`, as it's not used and it doesn't work now since you modify the package name.
--
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] [skywalking-satellite] EvanLjp commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
EvanLjp commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853677735
> Now the import path in this repo is `github.com/apache/skywalking-satellite/protocol/gen-codes/satellite/protocol`, I think adding `satellite` folder in `goapi` repo is enough, now
add a proto file in go-API rather than download the proto file?
--
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] [skywalking-satellite] wu-sheng commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853642767
The next step should move the dependency to `skywalking-goapi` repo, @EvanLjp gets blocked by his network issue.
--
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] [skywalking-satellite] wu-sheng merged pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43
--
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] [skywalking-satellite] wu-sheng commented on a change in pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#discussion_r644552403
##########
File path: Makefile
##########
@@ -54,15 +54,6 @@ tools:
deps: tools
$(GO_GET) -v -t -d ./...
-.PHONY: gen-codes
-gen-codes:
- $(PROTOC) --version || sh tools/install_protoc.sh
- /bin/sh tools/protocol_gen.sh
-
-.PHONY: gen-docs
-gen-docs: build
- rm -rf $(PLUGIN_DOC_DIR)
- $(OUT_DIR)/$(BINARY)-$(VERSION)-$(OSNAME)-$(ARCH) docs --output=$(PLUGIN_DOC_DIR)
Review comment:
One file wouldn't be able to be removed, satellite has its own event proto. But we should remove most of others, and host the generated source codes directly to avoid the complexity.
--
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] [skywalking-satellite] wu-sheng commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853657210
Consider adding `satellite/internal` folder into goapi repo, and move proto with generated source codes there. Is this better?
--
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] [skywalking-satellite] wu-sheng commented on a change in pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#discussion_r644552403
##########
File path: Makefile
##########
@@ -54,15 +54,6 @@ tools:
deps: tools
$(GO_GET) -v -t -d ./...
-.PHONY: gen-codes
-gen-codes:
- $(PROTOC) --version || sh tools/install_protoc.sh
- /bin/sh tools/protocol_gen.sh
-
-.PHONY: gen-docs
-gen-docs: build
- rm -rf $(PLUGIN_DOC_DIR)
- $(OUT_DIR)/$(BINARY)-$(VERSION)-$(OSNAME)-$(ARCH) docs --output=$(PLUGIN_DOC_DIR)
Review comment:
One file wouldn't be able to be removed, satellite has its own event proto. But we should remove most of others, and host the generated source codes directly to avoid the complexity.
--
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] [skywalking-satellite] EvanLjp commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
EvanLjp commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853653628
would add satellite gen-codes in go-api. I would continue to do these works at night.
--
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] [skywalking-satellite] kezhenxu94 commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853680274
> > Now the import path in this repo is `github.com/apache/skywalking-satellite/protocol/gen-codes/satellite/protocol`, I think adding `satellite` folder in `goapi` repo is enough, now
>
> add a proto file in go-API rather than download the proto file?
Create a folder `satellite` in goapi repo, and add the proto files (i.e. `Event.proto`), and their generated `.go` files there. Then we import the goapi module in this repo.
--
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] [skywalking-satellite] EvanLjp commented on pull request #43: change go api package
Posted by GitBox <gi...@apache.org>.
EvanLjp commented on pull request #43:
URL: https://github.com/apache/skywalking-satellite/pull/43#issuecomment-853653628
--
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