-
Notifications
You must be signed in to change notification settings - Fork 800
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
Adding make file to generate allocation go from proto #1041
Conversation
Build Succeeded 👏 Build Id: 99b4dc9b-eec9-417a-ac77-d0389d63d315 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@markmandel can you please review? |
Build Succeeded 👏 Build Id: 34922904-55b5-49a6-b58f-6317e31dfc3d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
9ea12c0
to
518062a
Compare
Build Succeeded 👏 Build Id: 175d1e69-c3c1-40d6-9bc1-8c957b3a75be The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 32600357-e949-4a8b-9408-6ef5818e8807 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 087667f0-5ab1-46aa-afe7-90c22ee95448 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Will jump on this next week, I know you are waiting on it 👍 |
build/includes/allocation.mk
Outdated
|
||
# Generates grpc server and client for a single allocation, use ALLOCATION_FOLDER variable to specify the allocation folder. | ||
gen-allocation-grpc: COMMAND := gen | ||
gen-allocation-grpc: run-allocation-command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this run the container with gen.sh? Do we need all the searching functionality in run-allocation-command
?
In this particular instance, it feels like it might be overkill? WDYT? Simpler may be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified it.
9a61337
to
d0e6cae
Compare
Build Failed 😱 Build Id: a2443464-7bab-49c2-a44e-41480077e5ba To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 3ea1dbc8-0cd9-4459-8085-e10525eb5e05 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
go install -mod=vendor github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway | ||
go install -mod=vendor github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger | ||
|
||
outputpath=./allocation/go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go under pkg/allocation/go
. The generated code for sdk.proto
doesn't live at the top level of the repo; it lives under the pkg directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks for the suggestion.
go install -mod=vendor github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger | ||
|
||
outputpath=./allocation/go | ||
protopath=cmd/allocator/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't have expected the proto source to be under cmd
. The cmd
directory should be for main program entry points, and the proto definition is something that generates shared code.
Now that we have multiple proto files, it seems like it would be nice to create a folder to hold them (e.g. proto/
) at the root level. The proto file isn't being added in the change, so I don't want to block you from making progress, but I do think we should find it a better home.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am addressing this comment in: #1041
I will update the protopath reference after the PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Build Succeeded 👏 Build Id: 3ec7480e-cb48-4425-9505-ecb2fc57ebc1 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 23384ec4-33e5-4649-b410-60b8cb4b7021 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: ff0d29dd-b55f-4eb5-9530-5a1f48b3b84f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pooneh-m, roberthbailey The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Build Succeeded 👏 Build Id: 5f20e53c-5af9-4537-8cc6-278c0603cc0d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
The change is following the SDK generation Docker and make files and referencing build-sdk-images/tool/.. which should be refactored. Refactoring is not included in this PR to simplify the review.
issue: #597