VPA: Enforce flag documentation check in CI #7857

pjsharath28 · 2025-02-23T14:05:26Z

What type of PR is this?

/area vertical-pod-autoscaler
/kind cleanup

What this PR does / why we need it:

Adds a CI check to enforce that VPA flag documentation remains up-to-date by failing if modifications are detected.

Which issue(s) this PR fixes:

Fixes #7844

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-02-23T14:05:34Z

Welcome @pjsharath28!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-02-23T14:05:35Z

Hi @pjsharath28. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

adrianmoisey · 2025-02-23T18:15:10Z

/ok-to-test

omerap12

Thanks for the PR!
Please check my attached comment. I realize I wasn’t clear enough in the issue description - sorry about that.
Could you add a script to the autoscaler/hack directory that triggers vertical-pod-autoscaler/hack/verify-vpa-flags.sh? (verify-all.sh calls all verify-* scripts in this directory.)"

omerap12 · 2025-02-24T07:02:23Z

.github/workflows/ci.yaml

+      - name: Verify vertical-pod-autoscaler flags
+        working-directory: ${{ env.GOPATH }}/src/k8s.io/autoscaler/vertical-pod-autoscaler
+        run: hack/verify-vpa-flags.sh
+        env:
+          GO111MODULE: auto
+


This is unnecessary since the previous step already runs all verify-*.sh files.

It's not getting considered probably because it is in a different directory.
I'll update the verify-all.sh so that this block could be removed.

Yes, as I said you need to add a script to the autoscaler/hack directory that triggers vertical-pod-autoscaler/hack/verify-vpa-flags.sh

Hey @omerap12, I made changes as discussed.
But the already existing vertical-pod-autoscaler/hack/verify-codegen.sh is failing here
Kindly let me know how we could proceed further.

The script vertical-pod-autoscaler/hack/verify-codegen.sh runs because of your change in hack/verify-all.sh. But we don’t want to run all verify-* scripts in the vertical-pod-autoscaler/hack folder.

I suggest adding a new script, like verify-vpa-flags.sh, in the main hack folder. This script will run vertical-pod-autoscaler/hack/verify-vpa-flags.sh.

Updated. PTAL!

This still needs to be called from the GitHub Action though, which doesn't happen at the moment

My bad! I see we have this:

autoscaler/hack/verify-all.sh

Line 56 in c5789ae

for t in `ls $KUBE_ROOT/hack/verify-*.sh`

omerap12 · 2025-02-24T07:15:31Z

vertical-pod-autoscaler/hack/verify-vpa-flags.sh

+if [[ "$existing_flags" == "$updated_flags" ]]; then
+    echo "VPA flags are up to date."
+else
+    echo "VPA flags have been modified."
+    exit 1
+fi


There's no need to print anything when the check passes. I suggest using something like this instead:

if [[ "$existing_flags" != "$updated_flags" ]]; then echo "VPA flags are not up to date. Please run ${GENERATE_FLAGS_SCRIPT}" exit 1 fi

k8s-ci-robot · 2025-02-24T11:12:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pjsharath28
Once this PR has been reviewed and has the lgtm label, please assign kgolab, voelzmo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

hack/OWNERS
vertical-pod-autoscaler/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

omerap12

Just a small nit.

omerap12 · 2025-02-25T07:40:12Z

hack/verify-vpa-flags.sh

+echo "Running VPA flags verification..."
+"$VPA_FLAGS_SCRIPT"
+
+echo "VPA flags verification completed successfully."


nit: no need to print this - we just need to ensure the pipeline passes

omerap12 · 2025-02-25T10:51:15Z

/lgtm
Pinging @voelzmo and @adrianmoisey for second review.

adrianmoisey · 2025-02-25T11:01:29Z

/lgtm

Thanks for doing this!

k8s-ci-robot added area/vertical-pod-autoscaler kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Feb 23, 2025

k8s-ci-robot requested review from gjtempleton and jbartosik February 23, 2025 14:05

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 23, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 23, 2025

pjsharath28 marked this pull request as draft February 23, 2025 14:15

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2025

pjsharath28 force-pushed the vpa-flag-documentation-enforcement branch 2 times, most recently from 933eb01 to ab14079 Compare February 23, 2025 16:03

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2025

pjsharath28 force-pushed the vpa-flag-documentation-enforcement branch from ab14079 to 7d4a883 Compare February 24, 2025 04:09

pjsharath28 marked this pull request as ready for review February 24, 2025 04:29

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2025

k8s-ci-robot requested review from kwiesmueller and MaciekPytel February 24, 2025 04:29

omerap12 reviewed Feb 24, 2025

View reviewed changes

pjsharath28 force-pushed the vpa-flag-documentation-enforcement branch 2 times, most recently from bb442f2 to e752f67 Compare February 24, 2025 11:12

pjsharath28 force-pushed the vpa-flag-documentation-enforcement branch from e752f67 to 73fa3ed Compare February 25, 2025 06:13

pjsharath28 requested a review from omerap12 February 25, 2025 06:24

omerap12 reviewed Feb 25, 2025

View reviewed changes

VPA: Enforce flag documentation check in CI

21316a6

pjsharath28 force-pushed the vpa-flag-documentation-enforcement branch from 73fa3ed to 21316a6 Compare February 25, 2025 10:11

pjsharath28 requested a review from omerap12 February 25, 2025 10:11

k8s-ci-robot assigned omerap12 Feb 25, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2025

k8s-ci-robot assigned adrianmoisey Feb 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPA: Enforce flag documentation check in CI #7857

VPA: Enforce flag documentation check in CI #7857

pjsharath28 commented Feb 23, 2025

k8s-ci-robot commented Feb 23, 2025

k8s-ci-robot commented Feb 23, 2025

adrianmoisey commented Feb 23, 2025

omerap12 left a comment

omerap12 Feb 24, 2025

pjsharath28 Feb 24, 2025 •

edited

Loading

omerap12 Feb 24, 2025

pjsharath28 Feb 24, 2025

omerap12 Feb 24, 2025

pjsharath28 Feb 25, 2025

adrianmoisey Feb 25, 2025

adrianmoisey Feb 25, 2025

omerap12 Feb 24, 2025

k8s-ci-robot commented Feb 24, 2025

omerap12 left a comment

omerap12 Feb 25, 2025

pjsharath28 Feb 25, 2025

omerap12 commented Feb 25, 2025

adrianmoisey commented Feb 25, 2025

VPA: Enforce flag documentation check in CI #7857

Are you sure you want to change the base?

VPA: Enforce flag documentation check in CI #7857

Conversation

pjsharath28 commented Feb 23, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

k8s-ci-robot commented Feb 23, 2025

k8s-ci-robot commented Feb 23, 2025

adrianmoisey commented Feb 23, 2025

omerap12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjsharath28 Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 24, 2025

omerap12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omerap12 commented Feb 25, 2025

adrianmoisey commented Feb 25, 2025

pjsharath28 Feb 24, 2025 •

edited

Loading