> 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 > >
