Conversation
benlwalker
commented
Oct 21, 2022
benlwalker
commented
- Remove all generated code from the storage part of the repo. It's generated, so it does not need to be version controlled.
- Simplify the Makefile by removing docker (protoc is packaged on most distros so it's trivial to install) and adding a convenience function.
|
I might need to have it generate code. Where exactly are you seeing an error? When building opi-spdk-bridge? Also I completely wrecked the existing storage github action and it needs to be rewritten another way. I'll work on that. |
Yes, when running go get and go build i see failure.
Sure, go ahead |
|
We also need to keep either v1 or v1alpha versioning somehow |
|
I can easily keep the v1 directory, but I did not understand what purpose that served in the repo itself. |
Like this example we have folders every time new api version released https://github.com/tektoncd/pipeline/tree/main/pkg/apis/pipeline At least this is my grpc versioning understanding |
|
v1 directory serves to version the APIs. Take a look at Uber's Protobuf Style Guide V2 and Google's API directory structure. Versioning and serving the generated artifacts makes it easier for Go developers to get versioned artifacts via |
glimchb
left a comment
glimchb
left a comment
There was a problem hiding this comment.
v1 folder and golang generations have to be resolved before merging this
benlwalker
force-pushed
the
storage
branch
2 times, most recently
from
980db82 to
e76d648
Compare
|
@glimchb After review of golang requirements and practices, I think the best approach is to have a dedicated, separate repository for the go bindings, and then sync to that using a github action from this repository. Are you able to create the go bindings repo? Do we need to discuss this somewhere and get everyone to agree? I can present pros and cons. |
|
no need to discuss, @benlwalker I agree |
|
I've reduced this PR just to the makefile cleanups. I'll do a separate PR for deleting the generated code once we have the dedicated repo set up. |
|
Did we decide to make a new repo to hold the golang bindings? What's the conclusion there? |
benlwalker
force-pushed
the
storage
branch
4 times, most recently
from
924f4db to
9767546
Compare
glimchb
left a comment
glimchb
left a comment
There was a problem hiding this comment.
looks better, thanks @benlwalker
This makes it easier to change in the future Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reduce the repeated code by using a function Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
If you want to build just one binding, or just the documentation, etc. now you can. This also makes parallel make work, so the whole thing builds much more quickly. Signed-off-by: Ben Walker <benjamin.walker@intel.com>
This is the same as any other generated output. Signed-off-by: Ben Walker <benjamin.walker@intel.com>
The same directory was mounted at /protos and /out. Only mount it once. Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
This will compile without any docker containers, using a locally installed protoc. BEWARE - every version of protoc generates different output, so don't check in generated output from this option. Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Don't lint anything generated Signed-off-by: Ben Walker <benjamin.walker@intel.com>
having client bindings in a separate repo would be good. |
intelfisz
left a comment
intelfisz
left a comment
There was a problem hiding this comment.
Are we waiting for GH actions adjustments to this PR yet or can it already be merged?
jainvipin
left a comment
jainvipin
left a comment
There was a problem hiding this comment.
this is a good positive change - only one comment, otherwise looks good to me. If this works for storage, we can adopt the same for networking as well.
| rm -rf ./v1alpha1/{autogen.md,gen,google} | ||
| mkdir -p ./v1alpha1/gen/{go,cpp,python,java} | ||
| CURRENT_VERSION := v1alpha1 | ||
| COMMON_VERSION := v1 |
There was a problem hiding this comment.
do we want to call it v1 as yet?
|
We have transitioned to using "buf" for generation of the protobuf bindings which are included in the makefiles. The plan is to move the bindings to a separate repo which is still being worked. Versioning of the various APIs needs to also be addressed in the current changes. The changes here need to be reviewed/updated for inclusion as needed as some of the changes are valuable. |
6 participants
