-
Notifications
You must be signed in to change notification settings - Fork 228
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
Delete ClusterQueue.ResourceGroups from cache_test #2545
Delete ClusterQueue.ResourceGroups from cache_test #2545
Conversation
/assign @mimowo |
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
LGTM, one question:
if all uses are removed, can we also remove the field itself? |
sorry, I meant the only test usage in this file /retest |
I see, so the current state is inconsistent since #1689 - the test cases specify the field, but it is ignored, we should definitely clean it up. I'm yet wondering if it is preferable to drop the asserts or revert the ignoring. Can you provide some justification one way or another? It seems reasonable to assert cache fields in |
We should drop these assets. While some of the assertions in the cache test make sense, I don't think they make sense for the capacity accounting fields. I think for the capacity related fields we should be testing against higher level concepts - such as, for some ResourceFlavor, |
Yeah, I can imagine maintaining too low-level tests might be a chore. For now, let's clean up (even if we end up asserting on ResourceGroups in the future), as in the current form the asserts are just confusing anyway. We will need to find a testing strategy while working on #79. /lgtm |
LGTM label has been added. Git tree hash: ddf214df1e4b772dfadcc92caa4d88363dbdb6b7
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabesaba, mimowo 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We removed the only test usage of this field in #2519 - see #2519 (comment). We are already ignoring it in the snapshot comparison.
Related to cleanup #2502
Special notes for your reviewer:
Does this PR introduce a user-facing change?