-
Notifications
You must be signed in to change notification settings - Fork 178
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
use all clusterqueues #3540
base: main
Are you sure you want to change the base?
use all clusterqueues #3540
Conversation
3b09d3d
to
01d5422
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.
Mostly LGTM, a few small things
(clusterQueues || []) | ||
.map((clusterQueue) => |
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.
You could instead get a combined array of all the resourceGroups
in all the ClusterQueues by doing:
(clusterQueues || []).flatMap((clusterQueue) => clusterQueue?.spec.resourceGroups || [])
And that would just replace the original (clusterQueue?.spec.resourceGroups || [])
, you'd chain the existing .reduce
on it and you wouldn't need your additional reduce at the end
const localQueues = useMakeFetchObject<LocalQueueKind[]>( | ||
useLocalQueues(namespace, refreshRate), | ||
); | ||
|
||
const clusterQueues = useMakeFetchObject<ClusterQueueKind[]>(useClusterQueues(refreshRate)); |
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.
This all looks good to me, but you'd want to add the logic you mentioned for filtering these by the namespace
right? You could rename this to allClusterQueues
and then have something like:
const clusterQueues = {
...allClusterQueues,
data: allClusterQueues.data.filter((clusterQueue) =>
localQueues.data.some(({ spec }) => spec.clusterQueue === clusterQueue.metadata.name),
),
};
You'll want to preserve the rest of the object you get back from useMakeFetchObject
so you're still passing down loading/error state, but you can override the data being included this way.
d66eda4
to
cf4222f
Compare
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
Signed-off-by: Kevin <[email protected]>
6f65e91
to
bac976a
Compare
Signed-off-by: Kevin <[email protected]>
bac976a
to
635df77
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3540 +/- ##
==========================================
- Coverage 85.19% 85.18% -0.01%
==========================================
Files 1382 1382
Lines 31572 31576 +4
Branches 8824 8827 +3
==========================================
+ Hits 26897 26899 +2
- Misses 4675 4677 +2
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mturley 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 |
/hold |
de565da
to
635df77
Compare
New changes are detected. LGTM label has been removed. |
https://issues.redhat.com/browse/RHOAIENG-8102
Description
With support for multiple clusterQueues, the first element can't be relied on for total shared quota
How Has This Been Tested?
I've used the existing Cyprus UI tests to verify that empty states still work as expected and I've added a unit test to ensure that when multiple CQs are used the total quota is correctly tallied up.
To manually test create two CQs and a LQ for each CQ and make sure that the total quota in the effected UI component is the sum of the quota for both CQs
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main