-
Notifications
You must be signed in to change notification settings - Fork 702
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
Ensure code generation dependencies are downloaded #2339
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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 updating this @astefanutti!
Makefile
Outdated
@@ -47,7 +47,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust | |||
output:rbac:artifacts:config=manifests/v2/base/rbac \ | |||
output:webhook:artifacts:config=manifests/v2/base/webhook | |||
|
|||
generate: controller-gen manifests ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. | |||
generate: go-mod-download controller-gen manifests ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. |
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.
Btw, I think you can remove controller-gen
task from the generate
, since manifests
runs controller-gen
:
Line 39 in 952f845
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. |
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.
Good point, updated.
Makefile
Outdated
@@ -47,7 +47,7 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust | |||
output:rbac:artifacts:config=manifests/v2/base/rbac \ | |||
output:webhook:artifacts:config=manifests/v2/base/webhook | |||
|
|||
generate: controller-gen manifests ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. | |||
generate: go-mod-download controller-gen manifests ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. |
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.
@astefanutti @kubeflow/wg-training-leads Do we need a separate task for go mod download
or we can just include this command as part of generate task, for instance:
generate: manifests ## Generate apidoc, sdk and code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
go mod download
$(CONTROLLER_GEN) object:headerFile="hack/boilerplate/boilerplate.go.txt" paths="./pkg/apis/..."
hack/update-codegen.sh
hack/python-sdk/gen-sdk.sh
hack/python-sdk-v2/gen-sdk.sh
$(MAKE) apidoc
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 find a separate target slightly more consistent with those for controller-get and Kustomize, but I don't have a strong opinion :)
Signed-off-by: Antonin Stefanutti <[email protected]>
Pull Request Test Coverage Report for Build 12089250799Details
💛 - Coveralls |
What this PR does / why we need it:
This PR runs
go mod download
prior to running code generation to ensure the required dependencies are present.Before:
After:
Checklist: