cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rohit Yadav" <bhais...@apache.org>
Subject Re: Review Request 17120: added an extra column to mark the imported ldap users as from ldap
Date Fri, 05 Dec 2014 18:27:52 GMT


> On Jan. 24, 2014, 6:52 a.m., Prachi Damle wrote:
> > I see many files unrelated to this fix have been updated and present in the patch.
> > Please can you update the patch with changes needed for the fix only?
> > 
> > Following are some such files.
> > api/src/com/cloud/user/AccountService.java
> > engine/schema/src/com/cloud/user/UserVO.java
> > plugins/dedicated-resources/test/org/apache/cloudstack/dedicated/manager/DedicatedApiUnitTest.java:
8 changes [ 1 2 3 4 5 6 7 8 ]
> > plugins/deployment-planners/implicit-dedication/test/org/apache/cloudstack/implicitplanner/ImplicitPlannerTest.java:
16 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 ]
> > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
> > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
> > 
> > Also the below file has all Mockito references replaced by Matchers - what is the
reason for this change?
> > plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java
> 
> Rajani Karuturi wrote:
>     The major changes for this patch are in the following files:
>     api/src/com/cloud/user/AccountService.java: 1 change [ 1 ] - > added a new overloaded
method to take the source of the account for the create call
>     api/src/com/cloud/user/User.java: 2 changes [ 1 2 ] -> added the new field's getter
and its enum
>     engine/schema/src/com/cloud/user/UserVO.java: 4 changes [ 1 2 3 4 ] -> mapped
the new database cloumn and added getter/setter, also changed the constructor to take the
new field
>     plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java:
2 changes [ 1 2 ] -> this is the one which is called when creating an ldap user and this
calls create account with source as LDAP
>     plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java:
2 changes [ 1 2 ] -> this is ldap's import multiple users command. change is same as above
>     server/src/com/cloud/user/AccountManagerImpl.java: 6 changes [ 1 2 3 4 5 6 ] ->
implementation of the new method
>     setup/db/db/schema-421to430.sql: 2 changes [ 1 2 ] -> SQL to add the new column
>     
>     The rest changes in tests/mock classes are due to change in constructor of UserVo
which cant be avoided
>     VmwareDatacenterApiUnitTest also has the same change as above. I dont see the Mockito/Matchers
change you mentioned. https://reviews.apache.org/r/17120/diff/#5
>     
>     Also note that patch1/diff1 is for 4.3 and and patch2/diff2 is for master. As we
are getting conflicts due to spaces changes I created two patches. The changes are same in
both the patches.
>     
>
> 
> Prachi Damle wrote:
>     You can avoid changing all the tests/mock classes. Don't change the existing UserVo
 constuctor - in the existing constructor default to source NATIVE so that no other callers
are changed.
>     Add a new constructor to take in source.
>      
>     What about existing ldap users on some existing setup - is there a way to identify
those? We should add upgrade steps to update the 'source' for those user records
>     
>
> 
> Rajani Karuturi wrote:
>     I followed this approach for AccountService. The changes to the UserVo constructor
mainly changed only the test files. the tests passed before and after the changes. I dont
see a reason why we should avoid changing tests/mocks. 
>     
>     There is no way to identify the existing ldap users in the current scenario. This
change helps us do that from this release.
> 
> Prachi Damle wrote:
>     Yes same approach as AccountService. I thought you said you cant avoid changing other
classes due to UserVO change. 
>     In general the reason is to avoid breaking any integration elsewhere and since you
already have a known default value to set, this is a way to avoid changes in places other
than your usecase. Ofcourse, not necessary to do it here considering its test code.
>     
>     
>     Provided this does not need to be supported backwards, then no upgrade is fine too.
>     
>     
>     
>     
>     
>     
>     
>     
>     
>     
>     
>     
>
> 
> Rajani Karuturi wrote:
>     Hi Prachi,
>     If you are fine with changes, can you push this commit? 
>     
>     Thanks,
>     Rajani

Hi, any update on this?

In case of SAML, I simply use a different namespace in UUID that starts with saml://, that
helps to identify that the user is authenticated/create by a SAML authentication. I used this
to avoid changing the schema at all.


- Rohit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17120/#review32702
-----------------------------------------------------------


On Aug. 13, 2014, 10:31 a.m., Rajani Karuturi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17120/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2014, 10:31 a.m.)
> 
> 
> Review request for cloudstack, Abhinandan Prateek, Ian Duffy, Min Chen, and Prachi Damle.
> 
> 
> Bugs: CLOUDSTACK-5910
>     https://issues.apache.org/jira/browse/CLOUDSTACK-5910
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> added an extra column to mark the imported ldap users as from ldap
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/user/AccountService.java a9be292 
>   api/src/com/cloud/user/User.java 36e9028 
>   engine/schema/src/com/cloud/user/UserVO.java 68879f6 
>   plugins/dedicated-resources/test/org/apache/cloudstack/dedicated/manager/DedicatedApiUnitTest.java
213174b 
>   plugins/deployment-planners/implicit-dedication/test/org/apache/cloudstack/implicitplanner/ImplicitPlannerTest.java
4182193 
>   plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java
4a00489 
>   plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
2f81688 
>   plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
100ffe6 
>   plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java
89cec65 
>   server/src/com/cloud/user/AccountManagerImpl.java 5204589 
>   server/test/com/cloud/configuration/ConfigurationManagerTest.java f1bb30a 
>   server/test/com/cloud/network/DedicateGuestVlanRangesTest.java 1615b84 
>   server/test/com/cloud/user/MockAccountManagerImpl.java 62e7fc8 
>   server/test/com/cloud/vm/UserVmManagerTest.java 83f7520 
>   server/test/com/cloud/vpc/NetworkACLManagerTest.java 629afa3 
>   server/test/com/cloud/vpc/NetworkACLServiceTest.java 786789f 
>   server/test/org/apache/cloudstack/affinity/AffinityApiUnitTest.java 061fd42 
>   server/test/org/apache/cloudstack/network/lb/CertServiceTest.java a67a9ab 
>   server/test/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServiceImplTest.java
d152c66 
>   setup/db/db/schema-421to430.sql ccff7c1 
> 
> Diff: https://reviews.apache.org/r/17120/diff/
> 
> 
> Testing
> -------
> 
> manually tested
> 
> 
> Thanks,
> 
> Rajani Karuturi
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message