You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by pedro-martins <gi...@git.apache.org> on 2016/03/10 19:27:31 UTC
[GitHub] cloudstack pull request: removed unused HypervDummyResourceBase cl...
GitHub user pedro-martins opened a pull request:
https://github.com/apache/cloudstack/pull/1437
removed unused HypervDummyResourceBase class
Was removed the class com.cloud.hypervisor.hyperv.resource.HypervDummyResourceBase
- This class do not have the annotation @Component and there are no beans that instantiate this class in XML files, then this class isn't in the spring lifecycle;
- The constructor of this class isn't called in the entire ACS code;
- There is no other class that extends this class.
Well, I think that this class isn't used in the ACS code, but if I'm wrong, please be free to comment.
Ty.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pedro-martins/cloudstack removed-unused-HypervDummyResourceBase-class
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1437.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 #1437
----
commit e1a75de9bc46776390d05aa1e996861a5a1e053a
Author: pedro-martins <ph...@gmail.com>
Date: 2016-03-10T17:46:33Z
removed unused
com.cloud.hypervisor.hyperv.resource.HypervDummyResourceBase
class
----
---
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 unused HypervDummyResourceBase cl...
Posted by anshul1886 <gi...@git.apache.org>.
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-221841462
LGTM. This file seems to be remains from the previous attempt of HyperV support which was talking to SCVMM.
---
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 unused HypervDummyResourceBase cl...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-216291321
I don't have the ability to test HyperV. Can anyone verify this PR for me?
---
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 unused HypervDummyResourceBase cl...
Posted by pedro-martins <gi...@git.apache.org>.
Github user pedro-martins commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195361064
@rafaelweingartner ty for your review.
Changed the log message.
Ty.
---
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 unused HypervDummyResourceBase cl...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-221871305
I think the only thing we are missing with this one is verification that the change works and does not break anything. @pedro-martins is there anything you can provide to show this is working?
---
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 unused HypervDummyResourceBase cl...
Posted by pedro-martins <gi...@git.apache.org>.
Github user pedro-martins commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195364975
Hi @eriweb, thanks for reviewing this PR.
I did a maven install in the ACS project and it built and ran all tests cases with no errors. But I didn't the integration test.
Ty.
---
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 unused HypervDummyResourceBase cl...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195095474
@eriweb
There are things that do not need to be tested, we work in computer science; if there is no reference to an object/class, there is no way it is being used.
The only reference for that class name is in a log at line 250 of class “com.cloud.hypervisor.hyperv.discoverer.HypervServerDiscoverer”, the log says that it is creating the dummyResource, however, it creates the “com.cloud.hypervisor.hyperv.resource.HypervDirectConnectResource” at line 282.
This is a fair and simple removal of unused code/class. @pedro-martins , could you also fix the log message in “HypervServerDiscoverer” at line 250?
---
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 unused HypervDummyResourceBase cl...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195620877
@remibergsma,
Let’s be reasonable, when you remove a method/class or something else, that do not have any reference anywhere in the code, in my opinion we do not need functional tests (the tests you call integration).
When Jenkins is running the maven build, it is already running unit and integration test cases (test cases written in Java using mocks and other tricks to test the code). In my opinion that would be enough.
I respect a lot your work here, but I really do not want to prolong this discussion; it seems that it was taken too far.
Let’s wait until someone execute the functional tests with Marvin.
---
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 unused HypervDummyResourceBase cl...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195369454
@eriweb I saw the Jenkins build, and it was ok, that meant the code compiled and all of the tests cases (unit and integration tests) were executed properly.
I noticed that the functional tests with Marvin were not executed (there is not CI in place right now), but I did not find them needed for this kind of contribution.
Additionally, I know that mistakes can happen if the code is not properly tested and reviewed. I just said that it did not need functional testing after getting the code, and carefully checking it myself; that is how I found the only reference to that class in a log message.
---
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 unused HypervDummyResourceBase cl...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-216225500
A CI testing (one with HyperV) should confirm if this PR can be accepted
---
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 unused HypervDummyResourceBase cl...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195625848
I see your point @rafaelweingartner, no hard feelings here. If we simply always run the integration tests, there is no discussion whether or not we need to run them. Otherwise others will also state they don't need testing and we end up with all sorts of nasty bugs. Let someone verify it against a really hyperv box. And if we can't find anyone who cares to do this, I'll +1 a PR that removes hyperv all together ;-).
---
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 unused HypervDummyResourceBase cl...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195620065
@GabrielBrascher based on what tests? It was only compiled.
I'm with @eriweb here, this should not go in without testing. Ask on the ML if someone with HyperV can confirm it still works and run at least `vm_life_cycle` integration tests.
@rafaelweingartner "There are things that do not need to be tested"? C'mon please.
To be clear: -1:-1: on this until we have done integration testing.
---
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 unused HypervDummyResourceBase cl...
Posted by GabrielBrascher <gi...@git.apache.org>.
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195618530
Given that @pedro-martins had no problems on running tests, the code 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 unused HypervDummyResourceBase cl...
Posted by eriweb <gi...@git.apache.org>.
Github user eriweb commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195069715
Have you done any testing?
---
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 unused HypervDummyResourceBase cl...
Posted by eriweb <gi...@git.apache.org>.
Github user eriweb commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195361874
@rafaelweingartner that's not really an answer, and I stand my question.
Have you done any testing? does the code compile? does unit tests run?
The code removed might not be used, but in a big sized project like this assumption is the mother of (almost) all fuckups.
---
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 unused HypervDummyResourceBase cl...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-195626691
I also got your point ;). Once you open an exception, there are people that may try to abuse it.
Here we might be able to get our hands on Hyper-V license, I will see if next week I can get someone to create a host with Hyper-V and run the test you mentioned.
---
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 issue #1437: removed unused HypervDummyResourceBase class
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the issue:
https://github.com/apache/cloudstack/pull/1437
### ACS CI BVT Run
**Sumarry:**
Build Number 144
Hypervisor xenserver
NetworkType Advanced
Passed=72
Failed=1
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
* test_routers.py
* test_03_restart_network_cleanup Failed
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
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 unused HypervDummyResourceBase cl...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1437#issuecomment-219696525
@anshul1886 @anshulgangwar Can you review this?
---
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.
---