cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Prachi Damle" <prachi.da...@citrix.com>
Subject Re: Review Request 17120: added an extra column to mark the imported ldap users as from ldap
Date Thu, 30 Jan 2014 01:20:10 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.

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.


- Prachi


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


On Jan. 22, 2014, 7:24 a.m., Rajani Karuturi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17120/
> -----------------------------------------------------------
> 
> (Updated Jan. 22, 2014, 7:24 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