[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-218959384
  
LGTM (just code review), based on what @anshul1886 says there should not be 
backward compatibility issue though I've not verified this by performing manual 
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-218462063
  
@anshul1886 I think the ask is that it get extracted as a method and a test 
be written for it.  @pedro-martins and @alexandrelimassantana, I feel that this 
might be a bit overkill.  Can you explain what the test would be validating?

As for the backwards compatibility issue, I am not entirely sure I 
understand the concern.  Can someone help me understand what that concern is?  
Thx...  


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[anshul1886]

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-218381704
  
I don't find much value here in adding documentation as code seems to be 
self explanatory. Regrading backward compatibility I have already answered in 
my previous comment.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-218367282
  
@anshul1886 can you please review and address the comments in this PR?  
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[alexandrelimassantana]

Github user alexandrelimassantana commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-217842674
  
as @pedro-martins stated, it seems to be fitting that this method is 
extracted to a class to be documented/tested. The code looks good but if the 
exception is to be launched I see no room for backward compatibility. If that's 
an issue, this needs to be rethinked as the raised exception would have to be 
treated and at that point we would have to think if raising the exception was 
really necessary. Overall the code looks good, simple and justified.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[anshul1886]

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-217610090
  
@rhtyd @swill There will not be backward compatibility issues as with 
static offering those parameters were not taken into consideration. They were 
wrongly giving the impression that they are being used.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-217480884
  
@anshul1886 can you answer @rhtyd's question so we can get his code review 
and get this moving forward.  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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[rhtyd]

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-216219348
  
@anshul1886 please rebase against latest master, can you explain if this 
can cause backward compatiblity issue

tag:easypr


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[swill]

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-215080256
  
Looking for one more LGTM for this one...  Thx...


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[koushik-das]

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-215070344
  
LGTM. Verified on a simulator setup. Also the test failures from CI run are 
unrelated to this change.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[bvbharatk]

Github user bvbharatk commented on the pull request:

https://github.com/apache/cloudstack/pull/1280#issuecomment-202622942
  
### ACS CI BVT Run
 **Sumarry:**
 Build Number 141
 Hypervisor xenserver
 NetworkType Advanced
 Passed=101
 Failed=5
 Skipped=4

_Link to logs Folder (search by build_no):_ 
https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0


**Failed tests:**
* integration.smoke.test_loadbalance.TestLoadBalance

 * test_01_create_lb_rule_src_nat Failed

 * test_02_create_lb_rule_non_nat Failed

 * test_assign_and_removal_lb Failed

* integration.smoke.test_volumes.TestCreateVolume

 * test_01_create_volume Failed

 * test_06_download_detached_volume Failed


**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_guest_vlan_range.TestDedicateGuestVlanRange
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9199: Fixed deployVirtualMachi...

[anshul1886]

GitHub user anshul1886 opened a pull request:

https://github.com/apache/cloudstack/pull/1280

CLOUDSTACK-9199: Fixed deployVirtualMachine API does not throw an error 
when cpunumber is specified for static compute offering

https://issues.apache.org/jira/browse/CLOUDSTACK-9199

To test:
-
Deploy VM by providing cpuNumber, cpuSpeed or memory for static/ non 
dynamic service offering
Deployment should fail.

API example:
--

http://10.220.135.6/client/api?command=deployVirtualMachine=olotwo==ab6e4154-62a3-42a8-9627-3cbdc66bcbb6=3ce6-91b4-11e5-b6fc-e26c2aa1d1d0=XenServer=39643075-4b45-489d-afac-88f09d536bdd[0].cpuNumber=1[0].cpuSpeed=1000[0].memory=1000=60844698-91b4-11e5-b6fc-e26c2aa1d1d0&_=1448277187743



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-9199

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1280.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 #1280


commit 149f6c05147a07abe845b53ed4d9bc159b68b0ff
Author: Anshul Gangwar 
Date:   2015-12-23T06:42:44Z

CLOUDSTACK-9199: Fixed deployVirtualMachine API does not throw an error 
when cpunumber is specified for static compute offering




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---