-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add UUID field for LDAP configuration #11462
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
Conversation
DaanHoogland
left a comment
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.
clgtm, but let’s do this in 4.20.2 or 4.22 ? including some of the other ldap improvements?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11462 +/- ##
============================================
+ Coverage 17.42% 17.57% +0.14%
- Complexity 15336 15493 +157
============================================
Files 5892 5894 +2
Lines 526521 528978 +2457
Branches 64293 65276 +983
============================================
+ Hits 91767 92964 +1197
- Misses 424401 425640 +1239
- Partials 10353 10374 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sureshanaparti
left a comment
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.
clgtm
...s/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapConfigurationVO.java
Show resolved
Hide resolved
|
@blueorgangutan package |
|
@blueorgangutan package |
|
@blueorangutan package |
|
@shwstppr 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 14961 |
|
@Pearl1594 , should this be a draft still? |
|
any particular reason this needs to be a draft @DaanHoogland . I believe it's ready for review and test. |
weizhouapache
left a comment
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.
code lgtm
…uuid-ldap-conf
I was asking because of the upgrade path |
right, I did not notice it at all 🤦 |
|
Updated the upgrade path :). Thanks @DaanHoogland |
DaanHoogland
left a comment
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.
clgtm
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@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 15168 |
|
@blueorangutan test keepEnv |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14449)
|
|
@blueorangutan package |
|
@vishesh92 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. |
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java
Show resolved
Hide resolved
...icators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java
Show resolved
Hide resolved
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15233 |
|
@blueorangutan test keepEnv |
|
@vishesh92 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
vishesh92
left a comment
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 did a cursory review of code and it looks good to me.
I created 2 LDAP configs in UI with same host & port but different domains.
After the fix, I am able to open the LDAP config I want and see the correct details.
|
[SF] Trillian test result (tid-14479)
|
...icators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java
Show resolved
Hide resolved
weizhouapache
left a comment
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.
code lgtm
|
merging based on approvals and manual test. |
|
* Add UUID field for LDAP configuration * move db changes to the lastest schema file * Add ID param to list ldapConf API & delete ldapConf API * fix ui test * fix 1 ui test * fix test * fix api description --------- Co-authored-by: dahn <[email protected]>


Description
This PR adds UUID for the ldap config, which will help resolve #11440
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?