-
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
VPA: fixed flags script to include constants values #7642
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 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 |
/lgtm |
Sorry for only catching up with this properly now, but I'm not sure this is the right approach. Having something different than |
Oh wow, yeah, that's a great idea! |
Is there any reason to not merge this now, and fix it later? |
Thanks for the reference! I can work on that. |
An idea: Once the script it setup, we could get it to run via GitHub Actions and either fail a PR if it doesn't update the docs, or it could run as a cron job, and make a PR if the flags in the documentation is wrong |
Yeah, we can probably just follow the pattern of |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR improves the VPA flag documentation generator script introduced in #7618. Previously, when flags had default values defined as constants (e.g., DefaultOOMBumpUpRatio), the script would output 0 as the default value in the generated documentation. For example:
Would result in showing 0 as the default value instead of the actual constant reference.
This PR modifies the script to display the constant reference (e.g., model.DefaultMemoryAggregationInterval) in the documentation rather than 0.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: