-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add Go control plane #103
Add Go control plane #103
Conversation
Note that the control plane needs to be built from the repository root, as: ``` docker build . -f docker/go-control-plane/Dockerfile ``` This is necessary to include to proto files into the build root.
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.
Haven't done a full pass yet. But I thought I'll just send whatever I have for now, and will send more comments when I find time to do a complete pass. Thanks.
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.
Thank you for the review! Did my best to address the comments.
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.
Thanks for the review. I did my best to address the comments.
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.
Thank you for the review. I addressed the comments.
Please note that I am working on this in parallel with #100. It is really difficult to implement changes that span both sides. I would like to check this in with the current proto files and then change the protocol once both components are checked in.
I can not implement final protocol now because I need the Python side (#100) completely setup in this repo to get access to some custom packages.
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.
Thanks for the review! I think it is all coming together. 2 things I would like to point out:
- Some comments I did not implement because of YAGNI. I would like to focus on merging the implementation for fallback test and not speculate about the future requirements.
resource.go
will be removed after I implement the new API (I need to merge first).
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.
Thanks for the review. Comments addressed.
89966aa
to
f656375
Compare
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.
Thanks. Addressed the comments.
Note that the control plane needs to be built from the repository root, as:
This is necessary to include to proto files into the build root.