You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by cristofolini <gi...@git.apache.org> on 2015/12/19 19:47:37 UTC
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
GitHub user cristofolini opened a pull request:
https://github.com/apache/cloudstack/pull/1262
Removed unnecessary code from getGuestOsType in CitrixResourceBase
Considering that all mapping of Guest OS Names to their respective hypervisor compatible types is made thorugh accessing a database, we've decided to remove a bit of code in the XcpOssResource class which was doing that same thing for 2 different OS's (both of which ARE in the database). That has led us to a bunch of unused parameters in the getGuestOsType method from its superclass, which we've also decided to remove. Test cases were added for four different possibilities for the platformEmulator String: one for a null String, one for a blank String, one for an empy String and one for a random case with a valid String.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rafaelweingartner/cloudstack lrg-cs-hackday-015
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1262.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1262
----
commit 0e97ca62190caebcc12cf2f889aeb2ebd950fd34
Author: cirstofolini <lu...@gmail.com>
Date: 2015-12-19T18:42:52Z
Removed unnecessary code from getGuestOsType in CitrixResourceBase
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208329721
Will, I don't like manic squashing, why not preserve history.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211431496
In this situation, I do not know if mine would count, giving that I also have helped with some code changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208326140
I am going to add this to my CI queue. Please reconfirm the LGTM's votes based on the updated code. Thanks...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208328877
Can I get the commits squashed and repushed with `push -f`. Thanks...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208071389
@DaanHoogland Done.
What do you think now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208327658
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208064343
Got your point.
@cristofolini is a little without time now, so I will do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-203569921
@cristofolini, I can give an LGTM for this PR.
Thanks for the tests and the removal of unused code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208397525
@DaanHoogland, I agree with Will. The last commit should be squashed. I will do that.
@swill I would not like to give an LGTM here, because @cristofolini is my colleague and I have helped him with the code. Maybe someone else could review the code?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-207987383
@rafaelweingartner how about creating a test-child instead of testing in all child-tests ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208059482
@DaanHoogland, I did not understand what you meant with test-child.
We could do the test a little bit different if we had a newer version of Mockito using the spy method. But, with the one we are using, we did not find much ways to write tests for that class, given that it is an abstract class.
Could you elaborate a little bit more your idea?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211436333
@rafaelweingartner I understand you feel like you have a conflict of interests, but at the same time, you are also very familiar with the code so are in a good position to review it. If you trust yourself to be critical of the code in your review, I trust your review. :P
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208401827
@DaanHoogland, @swill commits squashed.
I preserved the @cristofolini commit, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-207113402
@rafaelweingartner yes, I think this is ready too.
I am trying to get two LGTM for every PR, so can someone else from the dev list give me a review?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211438727
@swill thanks for the support.
I have already been very critical with this PR even before it was opened. So, the way it is now, it is good to be merged. There is only one thing I did not like at "CitrixResourceBaseTest .java", but as I discussed with Daan, we will be able to fix that once we use a newer version of Mockito.
I sure can give an LGTM to it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208064261
Well, I am not interested in elegant at this stage, just in reducing the code base if it make sense. The simplest solution is an named test resource class as child of CitrixResourceBase and test on that one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-207084566
@swill another one that seems to be ready to be merged.
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211441451
Ok perfect, thanks @rafaelweingartner. I will get this merged...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1262
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-203537918
### ACS CI BVT Run
**Sumarry:**
Build Number 148
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=0
Skipped=4
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso
**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208076663
I agree with you.
We did not use this solution at first because of this:
```
protected CitrixResourceBase citrixResourceBase = new CitrixResourceBase() {
@Override
protected String getPatchFilePath() {
return null;
}
};
```
Probably I was trying to be too perfectionist; I did not like that. Anyways, I hope we bump Mockito version soon, so I can fix that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-207460552
Do we need the tests in all child-test classes?
otherwise LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208347246
Some history is fine, but we don't need the whole history for every change in master.
I don't think this adds much by adding it to the master commit tree: https://github.com/apache/cloudstack/pull/1262/commits/c7b973972b65298030fae712afa6d50c146eaed8
Also, I need jenkins to run again, so I was killing two birds with one stone... :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-207133913
I agree with you
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208063842
Well @rafaelweingartner you can make a test class for each of the child classes of the abstract class, as done now or you can make a CitrixResourceTest-class child of the abstract class and test with that one. You could even make it an anonymous class in the test. I am not sure but maybe you can even mock/spy an abstract class and have the testable method be called without any child. Does that explain?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-166138005
@rafaelweingartner Done.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208064117
Sure it does.
And yes we can create a spy from an abstract class. The problem is that with the current version of Mockito the spy method only works for Objects and not for classes; we would need to create an anonymous class. Using the mock method would no work.
I also thought about using an anonymous class, but that seemed not elegant to me. However, if you find it interesting, we could do that way (with an anonymous class). Then, we would not even need to work with mocks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-208074579
@rafaelweingartner a lot less code (not in the diff but in the result) :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-207470025
@DaanHoogland that is a good question. The point here is that it is an abstract class being tested. So, instead of removing the abstract from that class, we tested the method in each of its implementations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-166032637
@cristofolini could you please remove the @Local annotation from CitrixResourceBase ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unnecessary code from getGuestOsT...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1262#issuecomment-211419101
I need one more LGTM code review of this one, otherwise it is looking to be in good order...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---