Add Protobuf generation to the codegen utility #1385
Conversation
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard
OR
Who should apply these qe/docs/px labels?
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
elmiko
left a comment
elmiko
left a comment
There was a problem hiding this comment.
code generally looks good to me, although i admit i'm not an expert on the go/protobuf tooling. i like all the comments, thanks for adding those.
i tried testing this by running make update-codegen from the root of a clean project, but i get this error
topological order github.com/openshift/api/apps/v1
2023/01/13 14:37:23 /home/mike/dev/openshift-api/github.com/openshift/api/apps/v1/generated.proto: Input is shadowed in the --proto_path by "github.com/openshift/api/apps/v1/generated.proto". Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.
2023/01/13 14:37:23 protoc -I . -I /home/mike/dev/openshift-api -I /home/mike/dev/openshift-api/vendor --gogo_out=/home/mike/dev/openshift-api /home/mike/dev/openshift-api/github.com/openshift/api/apps/v1/generated.proto
2023/01/13 14:37:23 Unable to generate protoc on github.com.openshift.api.apps.v1: exit status 1
did i do something wrong?
|
Interesting error! I don't think you did something wrong, but, are you using GOPATH? I believe that the generator doesn't currently work if you aren't in the standard GOPATH structure |
|
i notice my GOPATH is not set, so i set it and tried again but i get the same error: |
This isn't the right GOPATH structure expected by the tooling, you need |
|
i can try moving the openshift/api directory into my gopath, but i usually work on stuff outside the gopath. fwiw, my go path is like |
|
running from within the gopath did work |
Unfortunately, I know the protobuf generators aren't currently working outside of GOPATH, so I guess I could add a check in that warns about this 🤔 If you have it set up as expected, great, if you don't it causes errors as you've seen. Some of our other generators have a fix in for this but I'm waiting on upstream approval to fix the protobuf generator |
|
if the check isn't too annoying to write i'd say sure, otherwise i think it would be fine to just print a warning message when that command starts. then at least i know what i did wrong. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
the
lifecycle/stale
|
/lifecycle frozen |
|
@JoelSpeed: The In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
JoelSpeed
force-pushed
the
codegen-go-to-protobuf
branch
from
7987e3e to
4894dd2
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
added
the
size/XXL
|
@elmiko I've rebased this now and added two further commits, one updates the dependencies for the tools module, the second adds |
|
i tried this out on my local machine, outside of the GOPATH, by running it also appears to drop a |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
JoelSpeed
force-pushed
the
codegen-go-to-protobuf
branch
from
e0ebc62 to
af86c36
Compare
openshift-merge-robot
removed
the
needs-rebase
JoelSpeed
force-pushed
the
codegen-go-to-protobuf
branch
from
af86c36 to
c4f5259
Compare
JoelSpeed
force-pushed
the
codegen-go-to-protobuf
branch
from
c4f5259 to
fff23c1
Compare
|
|
||
| // Options contains the configuration required for the protobuf generator. | ||
| type Options struct { | ||
| // Disabled indicates whether the deepcopy generator is enabled or not. |
There was a problem hiding this comment.
| // Disabled indicates whether the deepcopy generator is enabled or not. | |
| // Disabled indicates whether the protobuf generator is enabled or not. |
| if os.Getenv("PROTO_OPTIONAL") != "" { | ||
| klog.Warningf("Skipping protobuf generation: %v", err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Should we do this at the beginning of this function? That way if PROTO_OPTIONAL is set we just don't do anything?
In the current state setting PROTO_OPTIONAL only skips generation if checking binaries returns an error?
There was a problem hiding this comment.
If the binaries are present, then PROTO_OPTIONAL shouldn't have an effect IMO. The PROTO_OPTIONAL was always a get out of jail free when folks didn't have the correct binaries. It has the side effect of skipping all protobuf generation, but the correct way to skip all protobuf generation would be to not run this generator. If the variable were SKIP_PROTO then I think it would make more sense.
This is a change of behaviour though, so maybe someone is relying on skipping the protobuf even when they do have the correct binaries 🤔
There was a problem hiding this comment.
Discussed sync, need to account for GOPATH and, if not in GOPATH, also skip. Should move proto optional to skip everything even if you have the binaries
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@JoelSpeed: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
This PR adds protobuf as a generator to the codegen generator utility.
The upstream generator unfortunately encodes a lot of logic within the cmd so the
Runwhich we are using useslog.Fatala lot when there are errors to return. For now, rather than duplicating the huge run fuction, I think we can work around that.I've set it up so that the protobuf generator is opt-in (which is different to our other generators) and that it will error if the dependent binaries are missing, but you can make that just a warning with
PROTO_OPTIONAL=1as you could before, and it will print this in the error messages.One thing that's new here, is the ability to disable a specific version of the API from generation, this is needed because the
imagegroup has proto generation on some versions, but not others. We may want to include this in other generators later but for now, it seems only to be needed on this generator.