API: Fix pagination issue with listing LDAP configuration - #11444
API: Fix pagination issue with listing LDAP configuration#11444Pearl1594 wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11444 +/- ##
=========================================
Coverage 16.35% 16.36%
- Complexity 13587 13593 +6
=========================================
Files 5669 5669
Lines 501474 501474
Branches 60913 60913
=========================================
+ Hits 82030 82066 +36
+ Misses 410251 410217 -34
+ Partials 9193 9191 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@pearl, It does not seem to work, I think we must consider the option ro add a uuid to the API/records |
|
Seemed to be a UI issue. It works via cmk. Thanks for testing @DaanHoogland |
|
| :columns="isOrderUpdatable() ? columns : columns.filter(x => x.dataIndex !== 'order')" | ||
| :dataSource="items" | ||
| :rowKey="(record, idx) => record.id || record.name || record.usageType || idx + '-' + Math.random()" | ||
| :rowKey="(record, idx) => hasNoUniqueKey() ? (idx + '-' + Math.random()) : (record.id || record.name || record.usageType || idx + '-' + Math.random())" |
There was a problem hiding this comment.
how about making the key/id be the concatenation of hostnam+”-“+port+”-“+domainid?
There was a problem hiding this comment.
good point @DaanHoogland
also, can we create a method for this ? @Pearl1594
There was a problem hiding this comment.
Does ldap configuration returns any of id, name or usagetype? Won't it automatically fallback to idx + '-' + Math.random()
On qa server, I was getting,
(qaserver) 🐱 > list ldapconfigurations
{
"LdapConfiguration": [
{
"hostname": "ldap.forumsys.com",
"port": 389
}
],
"count": 1
}
|
@Pearl1594 , the pagination works but no it reveals new issues, sorting does not work beyond the page we are looking at. I do not consider this a big issues as most installations will not have enormous amounts of ldap configurations, but still it seems like a(nother) justifications for giving ldapconfigurations a proper uuid. That said, lgtm. |
DaanHoogland
added
status:waiting-for-smoketest-results
and removed
needs-testing
labels
| :columns="isOrderUpdatable() ? columns : columns.filter(x => x.dataIndex !== 'order')" | ||
| :dataSource="items" | ||
| :rowKey="(record, idx) => record.id || record.name || record.usageType || idx + '-' + Math.random()" | ||
| :rowKey="(record, idx) => hasNoUniqueKey() ? (idx + '-' + Math.random()) : (record.id || record.name || record.usageType || idx + '-' + Math.random())" |
There was a problem hiding this comment.
good point @DaanHoogland
also, can we create a method for this ? @Pearl1594
41b92dd to
6f432d1
Compare
|
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
code lgtm
@Pearl1594 can you please check the outstanding comment?
| :columns="isOrderUpdatable() ? columns : columns.filter(x => x.dataIndex !== 'order')" | ||
| :dataSource="items" | ||
| :rowKey="(record, idx) => record.id || record.name || record.usageType || idx + '-' + Math.random()" | ||
| :rowKey="(record, idx) => hasNoUniqueKey() ? (idx + '-' + Math.random()) : (record.id || record.name || record.usageType || idx + '-' + Math.random())" |
There was a problem hiding this comment.
Does ldap configuration returns any of id, name or usagetype? Won't it automatically fallback to idx + '-' + Math.random()
On qa server, I was getting,
(qaserver) 🐱 > list ldapconfigurations
{
"LdapConfiguration": [
{
"hostname": "ldap.forumsys.com",
"port": 389
}
],
"count": 1
}
6f432d1 to
57f1529
Compare
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18756 |
|
@Pearl1594 , do you need to do more work on this (draft) PR? |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16697)
|
|
@Pearl1594 , this can be marked ready-for-merge right? |
57f1529 to
3546263
Compare
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM tested manually
Insertted the records in the database
mysql> INSERT INTO cloud.ldap_configuration (hostname, port, domain_id)
-> SELECT CONCAT('ldap-host-', LPAD(n, 2, '0')), 389, NULL FROM (
-> SELECT 1 n UNION SELECT 2 UNION SELECT 3 UNION SELECT 4 UNION SELECT 5
-> UNION SELECT 6 UNION SELECT 7 UNION SELECT 8 UNION SELECT 9 UNION SELECT 10
-> UNION SELECT 11 UNION SELECT 12 UNION SELECT 13 UNION SELECT 14 UNION SELECT 15
-> UNION SELECT 16 UNION SELECT 17 UNION SELECT 18 UNION SELECT 19 UNION SELECT 20
-> UNION SELECT 21 UNION SELECT 22 UNION SELECT 23 UNION SELECT 24 UNION SELECT 25
-> ) t;
Query OK, 25 rows affected (0.00 sec)
Records: 25 Duplicates: 0 Warnings: 0
[root@ref-trl-12348-k-Mol8-kiran-chavala-mgmt1 ~]# cmk list ldapconfigurations listall=true pagesize=20 page=1 filter=hostname,port
Loaded in-built API cache. Failed to read API cache, please run 'sync'.
{
"LdapConfiguration": [
{
"hostname": "10.0.33.32",
"port": 389
},
{
"hostname": "ldap-host-01",
"port": 389
},
{
"hostname": "ldap-host-02",
"port": 389
},
{
"hostname": "ldap-host-03",
"port": 389
},
{
"hostname": "ldap-host-04",
"port": 389
},
{
"hostname": "ldap-host-05",
"port": 389
},
{
"hostname": "ldap-host-06",
"port": 389
},
{
"hostname": "ldap-host-07",
"port": 389
},
{
"hostname": "ldap-host-08",
"port": 389
},
{
"hostname": "ldap-host-09",
"port": 389
},
{
"hostname": "ldap-host-10",
"port": 389
},
{
"hostname": "ldap-host-11",
"port": 389
},
{
"hostname": "ldap-host-12",
"port": 389
},
{
"hostname": "ldap-host-13",
"port": 389
},
{
"hostname": "ldap-host-14",
"port": 389
},
{
"hostname": "ldap-host-15",
"port": 389
},
{
"hostname": "ldap-host-16",
"port": 389
},
{
"hostname": "ldap-host-17",
"port": 389
},
{
"hostname": "ldap-host-18",
"port": 389
},
{
"hostname": "ldap-host-19",
"port": 389
}
],
"count": 26
}
[root@ref-trl-12348-k-Mol8-kiran-chavala-mgmt1 ~]# cmk list ldapconfigurations listall=true pagesize=20 page=2 filter=hostname,port
Loaded in-built API cache. Failed to read API cache, please run 'sync'.
{
"LdapConfiguration": [
{
"hostname": "ldap-host-20",
"port": 389
},
{
"hostname": "ldap-host-21",
"port": 389
},
{
"hostname": "ldap-host-22",
"port": 389
},
{
"hostname": "ldap-host-23",
"port": 389
},
{
"hostname": "ldap-host-24",
"port": 389
},
{
"hostname": "ldap-host-25",
"port": 389
}
],
"count": 26
}
UI
|
@Pearl1594 could please merge this pr |
@kiranchavala can I please request you for a quick check - I made a change to the UI, pertaining to rowkey to address the pending comment. Apologies. |
|
@Pearl1594 a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
|
7 participants
Description
This PR fixes issue with pagination when listing ldap configuration. When there are more than 20 entries and we want to list the 2nd page of ldap configs when using page size 20 , it returns the 1st 20 entries always.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?