You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Rajani Karuturi <ra...@gmail.com> on 2014/01/20 14:56:06 UTC
Review Request 17120: added an extra column to mark the imported ldap users
as from ldap
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17120/
-----------------------------------------------------------
Review request for cloudstack, Abhinandan Prateek and Ian Duffy.
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 8153a3f
api/src/com/cloud/user/User.java dcf27a0
engine/schema/src/com/cloud/user/UserVO.java 2a85758
plugins/dedicated-resources/test/org/apache/cloudstack/dedicated/manager/DedicatedApiUnitTest.java f2803b1
plugins/deployment-planners/implicit-dedication/test/org/apache/cloudstack/implicitplanner/ImplicitPlannerTest.java 24368ee
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java cdd883a
plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java 195e932
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 981e72e
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 1855d5d
server/src/com/cloud/user/AccountManagerImpl.java 0655f8c
server/test/com/cloud/configuration/ConfigurationManagerTest.java 2d2c03d
server/test/com/cloud/network/DedicateGuestVlanRangesTest.java ea7167b
server/test/com/cloud/user/MockAccountManagerImpl.java 38cc1a84
server/test/com/cloud/vm/UserVmManagerTest.java 34090ef
server/test/com/cloud/vpc/NetworkACLManagerTest.java 42619b2
server/test/com/cloud/vpc/NetworkACLServiceTest.java b17491a
server/test/org/apache/cloudstack/affinity/AffinityApiUnitTest.java 91d6958
server/test/org/apache/cloudstack/network/lb/CertServiceTest.java a37e626
server/test/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServiceImplTest.java 504ab9c
setup/db/db/schema-421to430.sql 8bfec37
Diff: https://reviews.apache.org/r/17120/diff/
Testing
-------
manually tested
Thanks,
Rajani Karuturi
Re: Review Request 17120: added an extra column to mark the imported
ldap users as from ldap
Posted by Rajani Karuturi <ra...@apache.org>.
> 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
>
> Rohit Yadav wrote:
> 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.
Hi Rohit,
these changes got merged with another security issue I was working and is under review. I added the extra column. I will discard this review request.
- Rajani
-----------------------------------------------------------
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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Prachi Damle <pr...@citrix.com>.
> 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.
>
>
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
- 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
>
>
Re: Review Request 17120: added an extra column to mark the imported
ldap users as from ldap
Posted by Rohit Yadav <bh...@apache.org>.
> 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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Rajani Karuturi <ra...@gmail.com>.
> 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.
>
>
>
>
>
>
>
>
>
>
>
>
>
Hi Prachi,
If you are fine with changes, can you push this commit?
Thanks,
Rajani
- Rajani
-----------------------------------------------------------
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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Prachi Damle <pr...@citrix.com>.
> 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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Rajani Karuturi <ra...@gmail.com>.
> 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
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.
- Rajani
-----------------------------------------------------------
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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Rajani Karuturi <ra...@gmail.com>.
> 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
>
>
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.
- Rajani
-----------------------------------------------------------
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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Prachi Damle <pr...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17120/#review32702
-----------------------------------------------------------
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
- Prachi Damle
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
>
>
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Rajani Karuturi <ra...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Rajani Karuturi <ra...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
updated with a patch for master
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 (updated)
-----
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
Re: Review Request 17120: added an extra column to mark the imported ldap
users as from ldap
Posted by Rajani Karuturi <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17120/
-----------------------------------------------------------
(Updated Jan. 22, 2014, 5:05 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 8153a3f
api/src/com/cloud/user/User.java dcf27a0
engine/schema/src/com/cloud/user/UserVO.java 2a85758
plugins/dedicated-resources/test/org/apache/cloudstack/dedicated/manager/DedicatedApiUnitTest.java f2803b1
plugins/deployment-planners/implicit-dedication/test/org/apache/cloudstack/implicitplanner/ImplicitPlannerTest.java 24368ee
plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java cdd883a
plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java 195e932
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java 981e72e
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java 1855d5d
server/src/com/cloud/user/AccountManagerImpl.java 0655f8c
server/test/com/cloud/configuration/ConfigurationManagerTest.java 2d2c03d
server/test/com/cloud/network/DedicateGuestVlanRangesTest.java ea7167b
server/test/com/cloud/user/MockAccountManagerImpl.java 38cc1a84
server/test/com/cloud/vm/UserVmManagerTest.java 34090ef
server/test/com/cloud/vpc/NetworkACLManagerTest.java 42619b2
server/test/com/cloud/vpc/NetworkACLServiceTest.java b17491a
server/test/org/apache/cloudstack/affinity/AffinityApiUnitTest.java 91d6958
server/test/org/apache/cloudstack/network/lb/CertServiceTest.java a37e626
server/test/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServiceImplTest.java 504ab9c
setup/db/db/schema-421to430.sql 8bfec37
Diff: https://reviews.apache.org/r/17120/diff/
Testing
-------
manually tested
Thanks,
Rajani Karuturi