-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix VPA code generation #6834
Fix VPA code generation #6834
Conversation
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.
Great! Left a couple comments 👍
1fa1df9
to
c3470b5
Compare
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.
LGTM!
rebased on recent master to re-trigger github action tests – somehow they were red although I didn't change anything related to the test? |
Turned out I was wrong: the boilerplate was missing for |
/assign kwiesmueller |
/assign @raywainman |
/lgtm Should we tweak the PR description to fix the following:
Looks like kube_codegen.sh takes care of a bunch of this for us, minus Some of my thoughts on the open questions:
Thanks @voelzmo! |
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nikimanoledaki, voelzmo 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 |
@raywainman I've rebase the PR and adapted the description a bit. Thanks for the review! I'll open follow-up PRs to remove the no longer supported API versions and for generating |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Seems like code generation wasn't working for a while, as the dependency for
hack/update-codegen.sh
wasn't vendored. This script wasn't executed in a while, although there were changes in the API recently. So in this PR, we switch to usingkube-codegen
instead of the deprecatedgenerate-groups
and make sure the dependencyk8s.io/code-generator
is actually installed.This PR proceeds step-wise, separated into individual commits, so it is more convenient to review:
hack/tools.go
to pull ink8s.io/code-generator
and have it correctly versioned ingo.mod
generate-groups.sh
fromall
toclient,deepcopy,informer,lister
. This excludesapplyconfiguration
, which we currently don't generate, but would be included inall
hack/update-codegen.sh
to generate the deepcopy files and client-related files with the most recent API changeskube-codegen.sh
, asgenerate-groups.sh
was showing a deprecation warninghack/update-codegen.sh
again, to see that there are no notable diffs after switching the generation script – the only changes seem to be concerned with the order of certain functions/variablesSpecial notes for your reviewer:
hack/verify-codegen.sh
a presubmit check?Does this PR introduce a user-facing change?