-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Update/verify codegen by make rulew #24252
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
Conversation
echo "Install codegen tools." | ||
cd "hack/tools" | ||
clientgen=${REPO_ROOT}/_bin/client-gen | ||
go build -o "${REPO_ROOT}/_bin/client-gen" k8s.io/code-generator/cmd/client-gen |
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 think it would make more sense to define make targets for these and make them dependencies of the update-codegen
target. That would also let us avoid rebuilding them when not needed.
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.
Not quite sure that I'm getting the idea. If you mean create a new make target just for installing tools used by update-codegen, it would work but I'm not sure it's the best idea since this new target is used only by update-codegen, maybe not worth the effort of inflating root level Makefile?
For local development, this is pretty fast after the first time, mostly due to go cache I think
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.
Ok, sounds good.
b1973f3
to
c546063
Compare
echo "Install codegen tools." | ||
cd "hack/tools" | ||
clientgen=${REPO_ROOT}/_bin/client-gen | ||
go build -o "${REPO_ROOT}/_bin/client-gen" k8s.io/code-generator/cmd/client-gen |
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.
Ok, sounds good.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chaodaiG, cjwagner 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 |
To make code reviewers' lives easier, this PR contains two commits: