Enable go multi-module workspace#4035
Conversation
7250fe2 to
1b4fe7f
Compare
|
/test pull-containerized-data-importer-fossa |
|
/test pull-containerized-data-importer-fossa |
|
/cc @akalenyu |
akalenyu
left a comment
There was a problem hiding this comment.
in general looks fine just worried about the sed dance around the group name
| ${SCRIPT_ROOT}/bin/openapi-spec-generator >${SCRIPT_ROOT}/api/openapi-spec/swagger.json | ||
|
|
||
| echo "************* running controller-gen to generate schema yaml ********************" | ||
| # controller-gen fails on vendored k8s.io/api/core/v1/doc.go due to empty +groupName= marker (see https://github.com/kubernetes/kubernetes/issues) |
There was a problem hiding this comment.
this was supposed to be a link to a particular issue or just the landing page?
There was a problem hiding this comment.
in any case how come we hit this here? api version bump? why not bump controller-gen in that case?
There was a problem hiding this comment.
yeah you are right, I updated codegen and the marker issue went away.
| @@ -0,0 +1,7 @@ | |||
| go 1.24.0 | |||
There was a problem hiding this comment.
I picked this line randomly to start the discussion. I noticed we are missing 2 things from the kubevirt referenced PR:
- bazel file changes (vendor->staging)
- symlink kubevirt/kubevirt@b2bbf1c#r2068133554 (mockgen)
I just want to make sure this is fine
There was a problem hiding this comment.
So the bazel file changes are related to client-go which we don't use, we use the runtime controller library instead which doesn't have the issue that is being solved. As for the symlink, I think that is fixing a specific kubevirt issue, that I don't think CDI has, so it didn't make sense to do that.
|
@Acedus I think you had a PR to kubevirt saving it from some go work consequence? |
Yeah it was a bug in go cmd that I fixed at the time: golang/go@1ebebf1 but I see that it only made it in 1.26, might be worth to see if my workaround applies here: kubevirt/kubevirt#14951 |
|
I just tried make clean with this PR and it worked without issues, maybe I am missing something, but everything appears to work as expected. |
| revert-deps-replace: | ||
| git checkout -- ${PWD}/vendor/k8s.io/api/core/v1/doc.go |
There was a problem hiding this comment.
we don't need this anymore right?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akalenyu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // +k8s:protobuf-gen=package | ||
| // +k8s:prerelease-lifecycle-gen=true | ||
| // +groupName= | ||
| // +groupName=core |
There was a problem hiding this comment.
guess you also want this reverted
|
are we missing the controller-gen bump in the builder? https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_containerized-data-importer/4035/pull-cdi-apidocs/2030977819466534912 |
|
Maybe, I guess I am making a new PR that bumps to controller gen version in the builder. |
|
Create a PR to update the builder #4051 |
Similar to KubeVirt kubevirt/kubevirt#14485 set up go workspace so we don't have the symlink to staging anymore. This will ensure hermeto no longer complains about changes to the modules.txt Signed-off-by: Alexander Wels <[email protected]>
Signed-off-by: Alexander Wels <[email protected]>
|
I think were missing a make generate and otherwise should be good to go |
controller gen Signed-off-by: Alexander Wels <[email protected]>
|
yeah, ran the make generate, should be good now. |
|
/lgtm |
|
/test pull-containerized-data-importer-fossa |
|
/cherry-pick release-v1.64 |
|
@awels: #4035 failed to apply on top of branch "release-v1.64": DetailsIn 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-sigs/prow repository. |
* Enable go multi-module workspace Similar to KubeVirt kubevirt/kubevirt#14485 set up go workspace so we don't have the symlink to staging anymore. This will ensure hermeto no longer complains about changes to the modules.txt Signed-off-by: Alexander Wels <[email protected]> * Re-add vendoring in containerized-data-importer-api Signed-off-by: Alexander Wels <[email protected]> * Updated the cdi builder to include the new controller gen Signed-off-by: Alexander Wels <[email protected]> --------- Signed-off-by: Alexander Wels <[email protected]>
What this PR does / why we need it:
Similar to KubeVirt kubevirt/kubevirt#14485 set up go workspace so we don't have the symlink to staging anymore. This will ensure hermeto no longer complains about changes to the modules.txt
Fixes https://issues.redhat.com/browse/CNV-60075
Special notes for your reviewer:
Release note: