-
Notifications
You must be signed in to change notification settings - Fork 4.2k
ci: dedicated UT jobs for CA and VPA #8669
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
ci: dedicated UT jobs for CA and VPA #8669
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis 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 |
8b43c4a to
d160e93
Compare
.github/workflows/ca-test.yaml
Outdated
| @@ -0,0 +1,41 @@ | |||
| name: Cluster Autoscaler Tests | |||
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.
A bit of a nit, could this be Cluster Autoscaler (and then VPA be Vertical Pod Autoscaler)
Then line 19 of this file gets changed to test (and similar in the VPA file).
Then make similar changes to the new vpa-golangci-lint.yaml file I made.
The reason is then this summary:
Will be:
Cluster Autoscaler / Test
Vertical Pod Autoscaler / Test
Vertical Pod Autoscaler / Lint
Making it super clear what is running
Then in the future when we break up verify too, we will have:
Cluster Autoscaler / Test
Cluster Autoscaler / Verify
Vertical Pod Autoscaler / Test
Vertical Pod Autoscaler / Verify
Vertical Pod Autoscaler / Lint
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.
+1 on that
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.
If we rename these "Cluster Autoscaler" and "Vertical Pod Autoscaler", then we are setting ourselves up to create monolith GitHub action definitions again (which we are trying to get away from). Are we confident that a single job for CA and VPA won't fill up storage like we recently encountered?
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.
If we rename these "Cluster Autoscaler" and "Vertical Pod Autoscaler", then we are setting ourselves up to create monolith GitHub action definitions again (which we are trying to get away from).
It's possible to have multiple GitHub actions with the same name, each one containing only a single job
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.
Done
| "build") | ||
| PROJECT="${1}" | ||
|
|
||
| case "${PROJECT}" in |
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.
Given that the logic is slightly different for each project anyway, I wonder if we should go a step further and just have a per-project file instead of keeping a single entry point here? If not, could we at least rename this file to something more accurate (e.g. run-unit-tests.sh)?
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'm fine with per-project files
These projects aren't really related at all, and only just share a repo.
Having to maintain the logic of handling multiple projects is sometimes annoying
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.
Agree on that
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'll do this in a follow-up PR so the changes can be more discrete to eval and potentially roll back
| "install") | ||
| "vertical-pod-autoscaler") | ||
| pushd ${CONTRIB_ROOT}/vertical-pod-autoscaler | ||
| go test -count=1 -race $(go list ./... | grep -v /vendor/ | grep -v vertical-pod-autoscaler/e2e | grep -v cluster-autoscaler/apis) |
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.
Hm, the cluster-autoscaler/apis shouldn't be relevant anymore, it only applied to CA. Same with vertical-pod-autoscaler/e2e and addon-resizer below.
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'll take this feedback and the feedback for per-project shell scripts as a follow-up.
|
I have 2 comments but they're more nits, don't block on them if needed. Adding a hold for other reviewers. /lgtm |
|
Bump |
c751abf to
adcd6bc
Compare
|
/label tide/merge-method-squash |
|
@adrianmoisey @towca this should be ready for a final review? I'll split out CA/VPA from the common shell scripts as follow-up tasks |
|
/lgtm Thanks! |
|
/hold cancel |
|
/cherry-pick cluster-autoscaler-release-1.34 |
|
@jackfrancis: #8669 failed to apply on top of branch "cluster-autoscaler-release-1.34": In response to this:
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. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR creates new
cluster-autoscaler-testandvertical-pod-autoscaler-testGitHub action jobs, and renamesverify-and-testto `verify.This will ensure we run only the tests we need for the changes in a particular PR, and should help prevent full disk errors from running a bunch of test and verify runs in a single job.
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.: