You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nvazquez <gi...@git.apache.org> on 2016/04/15 16:51:56 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9351: Add ids parameter to res...
GitHub user nvazquez opened a pull request:
https://github.com/apache/cloudstack/pull/1497
CLOUDSTACK-9351: Add ids parameter to resource listing API calls
## General behaviour
A new parameter is added in each method, its type a list of IDs of the entity, it will be mutually exclusive with id. (Similar to <code>id</code> and <code>ids</code> parameters in <code>listVirtualMachines</code> method)
### API Methods affected
* <code>listTemplates</code>: new parameter **<code>ids</code>**, mutually exclusive with <code>id</code>
* <code>listVolumes</code>: new parameter **<code>ids</code>**, mutually exclusive with <code>id</code>
* <code>listSnapshots</code>: new parameter **<code>ids</code>**, mutually exclusive with <code>id</code>
* <code>listVMSnapshots</code>: new parameter **<code>vmsnapshotids</code>**, mutually exclusive with <code>vmsnapshotid</code>
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nvazquez/cloudstack addidsparam
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1497.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 #1497
----
commit be4e51f98bb2c569f9182bdd3813888598324159
Author: nvazquez <ni...@gmail.com>
Date: 2016-04-12T17:57:55Z
CLOUDSTACK-9351: Add ids param to ListVolumesCmd
commit 5f9500550eef292665da9c90062f60fa5e611450
Author: nvazquez <ni...@gmail.com>
Date: 2016-04-14T14:39:41Z
CLOUDSTACK-9351: Add ids param to ListSnapshotsCmd
commit 8677458130437401e0c77f0fe672aa103778cb45
Author: nvazquez <ni...@gmail.com>
Date: 2016-04-14T15:28:22Z
CLOUDSTACK-9351: Add vmsnapshotids param to ListVMSnapshotsCmd
commit d2f22f0daa75f09412853917af49adab0a72d68a
Author: nvazquez <ni...@gmail.com>
Date: 2016-04-14T18:44:34Z
CLOUDSTACK-9351: Add ids param to ListTemplatesCmd
----
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r59917535
--- Diff: server/src/com/cloud/api/query/QueryManagerImpl.java ---
@@ -1790,6 +1802,10 @@
sc.setParameters("display", display);
}
+ if (ids != null && !ids.isEmpty()) {
--- End diff --
What about extracting line 1805 - 1808 to a method. Those lines are the same as the ones at 3172-3175. then, you could also write test cases and add java doc for 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211996504
Ok, I would modify javadoc and add a test for that particular case
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-217499701
@koushik-das @rhtyd I finished writing marvin test for this feature, I copy my test results:
````
[root@ussarlabcsmgt41 cloudstack]# nosetests --with-marvin --marvin-config=setup/dev/advanced.cfg test/integration/smoke/test_list_ids_parameter.py
==== Marvin Init Started ====
=== Marvin Parse Config Successful ===
=== Marvin Setting TestData Successful===
==== Log Folder Path: /tmp//MarvinLogs//May_06_2016_09_34_04_NHXZGI. All logs will be available here ====
=== Marvin Init Logging Successful===
==== Marvin Init Successful ====
===final results are now copied to: /tmp//MarvinLogs/test_list_ids_parameter_TRKKN0===
````
````
[root@ussarlabcsmgt41 cloudstack]# cat /tmp//MarvinLogs/test_list_ids_parameter_TRKKN0/results.txt
Test listing Volumes using 'ids' parameter ... === TestName: test_01_list_volumes | Status : SUCCESS ===
ok
Test listing Templates using 'ids' parameter ... === TestName: test_02_list_templates | Status : SUCCESS ===
ok
Test listing Snapshots using 'ids' parameter ... === TestName: test_03_list_snapshots | Status : SUCCESS ===
ok
Test listing VMSnapshots using 'vmsnapshotids' parameter ... === TestName: test_04_list_vm_snapshots | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 4 tests in 1187.175s
OK
````
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-210655468
@nvazquez you can create a common super class for those class, and put it into the “cloud-server” project. Additionally, you can remove the “extends ManagerBase”, I can bet it is not needed; so, let’s try removing some unused hierarchy?
Also, can you remove the "@Local" annotation? it is not needed.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218006600
I am missing one code review for this one. @nvazquez can you re-push to kick off Travis again? 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r60255522
--- Diff: server/test/com/cloud/api/query/MutualExclusiveIdsManagerBaseTest.java ---
@@ -0,0 +1,84 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+package com.cloud.api.query;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import com.cloud.utils.db.SearchCriteria;
+
+@RunWith(MockitoJUnitRunner.class)
+public class MutualExclusiveIdsManagerBaseTest {
+
+ @Mock
+ SearchCriteria<String> sc;
+
+ private static Long id1 = Long.valueOf(1l);
--- End diff --
here you could just use 1L.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218489998
Can you create a new PR to fix this and we will get it tested right away. Thanks for being on top of this. \U0001f44d
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218033521
I am a bit confused why the attributes tag of `advanced` did not work in my test and zero tests ran. I will look at that quickly tomorrow to see if there is something different about these tests from the others I am running this way. I think this one is in pretty good shape now. Will verify everything when I am back at a computer.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218466672
@swill Last Travis run for this PR was a success for test_list_ids_parameter. Which PR gives an issue?
==== Marvin Init Successful ====
=== TestName: test_01_list_volumes | Status : SUCCESS ===
=== TestName: test_02_list_templates | Status : SUCCESS ===
=== TestName: test_03_list_snapshots | Status : SUCCESS ===
===final results are now copied to: /tmp//MarvinLogs/test_list_ids_parameter_IPS7T4===
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211965259
That's cool! Thanks @swill!
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211585759
Cool, I'll work on 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211581223
@rafaelweingartner I pushed unit tests for this new methods.
So the idea is that the 3 classes extend from a new class and not ManagerBase?
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r59917122
--- Diff: server/src/com/cloud/api/query/QueryManagerImpl.java ---
@@ -1731,6 +1731,17 @@
Long zoneId = cmd.getZoneId();
Long podId = cmd.getPodId();
+ List<Long> ids = null;
--- End diff --
Would you mind extracting lines 1734-1744 to a method? Then, you could write test cases and even create some documentation for 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-216254374
Sure @rhtyd, I'll add them. I was working on it based on @koushik-das suggestion but I still have to work on 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211994713
The method name is ok. I just asked to confirm how you see the method workings. I only feel that the Javadoc should say that, whenever the ID variable is not null, it is the only one that is going to be used and returned in the List. Also, we should have a test for 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r62456186
--- Diff: test/integration/smoke/test_list_ids_parameter.py ---
@@ -0,0 +1,293 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+""" Tests for API listing methods using 'ids' parameter
+"""
+#Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import (cleanup_resources,
+ validateList)
+from marvin.lib.base import (Account,
+ Volume,
+ DiskOffering,
+ Template,
+ ServiceOffering,
+ Snapshot,
+ VmSnapshot,
+ VirtualMachine)
+from marvin.lib.common import (get_domain,
+ get_zone, get_template)
+from marvin.codes import FAILED, PASS
+from nose.plugins.attrib import attr
+#Import System modules
+import time
+
+_multiprocess_shared_ = True
+class TestListIdsParams(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ testClient = super(TestListIdsParams, cls).getClsTestClient()
+ cls.apiclient = testClient.getApiClient()
+ cls.services = testClient.getParsedTestDataConfig()
+ cls.hypervisor = testClient.getHypervisorInfo()
+
+ cls.domain = get_domain(cls.apiclient)
+ cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests())
+
+ cls.disk_offering = DiskOffering.create(
+ cls.apiclient,
+ cls.services["disk_offering"]
+ )
+
+ cls.account = Account.create(
+ cls.apiclient,
+ cls.services["account"],
+ domainid=cls.domain.id
+ )
+ cls.service_offering = ServiceOffering.create(
+ cls.apiclient,
+ cls.services["service_offerings"]["tiny"]
+ )
+
+ template = get_template(
+ cls.apiclient,
+ cls.zone.id,
+ cls.services["ostype"]
+ )
+ if template == FAILED:
+ assert False, "get_template() failed to return template with description %s" % cls.services["ostype"]
+
+ cls.services["template"]["ostypeid"] = template.ostypeid
+ cls.services["template_2"]["ostypeid"] = template.ostypeid
+ cls.services["ostypeid"] = template.ostypeid
+ cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+ cls.services["mode"] = cls.zone.networktype
+
+ #Create 3 VMs
+ cls.virtual_machine_1 = VirtualMachine.create(
+ cls.apiclient,
+ cls.services["virtual_machine"],
+ templateid=template.id,
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+ serviceofferingid=cls.service_offering.id,
+ mode=cls.services["mode"]
+ )
+ cls.virtual_machine_2 = VirtualMachine.create(
+ cls.apiclient,
+ cls.services["virtual_machine"],
+ templateid=template.id,
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+ serviceofferingid=cls.service_offering.id,
+ mode=cls.services["mode"]
+ )
+ cls.virtual_machine_3 = VirtualMachine.create(
+ cls.apiclient,
+ cls.services["virtual_machine"],
+ templateid=template.id,
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+ serviceofferingid=cls.service_offering.id,
+ mode=cls.services["mode"]
+ )
+
+ #Sleep for 12 minutes after vms creation because of bug while taking snapshots.
+ time.sleep(720)
+
+ #Take 3 VM1 Snapshots
+ cls.vmsnapshot_1 = VmSnapshot.create(
+ cls.apiclient,
+ cls.virtual_machine_1.id
+ )
+ cls.vmsnapshot_2 = VmSnapshot.create(
+ cls.apiclient,
+ cls.virtual_machine_1.id
+ )
+ cls.vmsnapshot_3 = VmSnapshot.create(
+ cls.apiclient,
+ cls.virtual_machine_1.id
+ )
+
+ #Stop VMs
+ cls.virtual_machine_1.stop(cls.apiclient)
+ cls.virtual_machine_2.stop(cls.apiclient)
+ cls.virtual_machine_3.stop(cls.apiclient)
+
+ #Get ROOT volumes of 3 VMs
+ vm1RootVolumeResponse = Volume.list(
+ cls.apiclient,
+ virtualmachineid=cls.virtual_machine_1.id,
+ type='ROOT',
+ listall=True
+ )
+ vm2RootVolumeResponse = Volume.list(
+ cls.apiclient,
+ virtualmachineid=cls.virtual_machine_2.id,
+ type='ROOT',
+ listall=True
+ )
+ vm3RootVolumeResponse = Volume.list(
+ cls.apiclient,
+ virtualmachineid=cls.virtual_machine_3.id,
+ type='ROOT',
+ listall=True
+ )
+ cls.vm1_root_volume = vm1RootVolumeResponse[0]
+ cls.vm2_root_volume = vm2RootVolumeResponse[0]
+ cls.vm3_root_volume = vm3RootVolumeResponse[0]
+
+ #Take 3 snapshots of VM2's ROOT volume
+ cls.snapshot_1 = Snapshot.create(
+ cls.apiclient,
+ cls.vm2_root_volume.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+ cls.snapshot_2 = Snapshot.create(
+ cls.apiclient,
+ cls.vm2_root_volume.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+ cls.snapshot_3 = Snapshot.create(
+ cls.apiclient,
+ cls.vm2_root_volume.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+
+ #Create 3 templates
+ cls.template_1 = Template.create(
+ cls.apiclient,
+ cls.services["template"],
+ cls.vm3_root_volume.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+ cls.template_2 = Template.create(
+ cls.apiclient,
+ cls.services["template_2"],
+ cls.vm3_root_volume.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+ cls.template_3 = Template.create(
+ cls.apiclient,
+ cls.services["template_2"],
+ cls.vm3_root_volume.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid
+ )
+
+ cls._cleanup = [
+ cls.disk_offering,
+ cls.account,
+ cls.service_offering,
+ cls.snapshot_1,
+ cls.snapshot_2,
+ cls.snapshot_3
+ ]
+
+ @classmethod
+ def tearDownClass(cls):
+ cls.apiclient = super(TestListIdsParams, cls).getClsTestClient().getApiClient()
+ try:
+ cleanup_resources(cls.apiclient, cls._cleanup)
+ except Exception as e:
+ raise Exception("Warning: Exception during cleanup : %s" % e)
+ return
+
+ def test_01_list_volumes(self):
--- End diff --
Add tags as appropriate
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211553575
@nvazquez thanks for the analysis on the ManagerBase issue.
Giving that I think the best approach would be to let all of the three (3) classes extending it, and then we could create a hierarchy, instead of using static methods.
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by serg38 <gi...@git.apache.org>.
Github user serg38 commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218480013
The issue that it couldn't find a snaphsot because it is already cleaned by account cleanup.
+ cls._cleanup = [
+ cls.disk_offering,
+ cls.account,
+ cls.service_offering,
+ cls.snapshot_1,
+ cls.snapshot_2,
+ cls.snapshot_3
+ ]
+
Snapshot cleanup can be safely removed since account cleanup will remove them automatically.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218047244
@swill As I had mentioned in my last comment, newly added tests are not tagged. The command you used to run these tests has tags, don't pass any tags in order to run them.
@nvazquez Please tag the tests appropriately, something like
@attr(tags = ["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false")
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218047917
@koushik-das actually I've added tags, removed time.sleep and commented out test_04
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1497
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218499768
Done @swill @serg38
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-212043816
Cool, I added both test cases
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-212008157
I included last changes and squashed all commits into 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211999079
Sorry if I am very picky sometimes; but, I believe that the more clear we are about a method behavior and the more tests to enforce that behavior we have, the better it is.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-212009376
That is great.
Now reading your documentation, I noticed another question.
And if both are null? Or if ID is null and IDs is empty? The way it is now, it will return a null or an empty list value.
Is that the expected behavior? If so, it deserves a test case.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-216228984
Nice PR, though this would need some regression tests. Can you add marvin smoke tests around list APis (add them to travis yml file)
tag:needlove
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-210561289
@nvazquez have you tested this change?
I noticed that you are using the SearchCriteria, shouldn’t that “idIN” have to be at the POJO and mapped with that name? How is that going to be transformed into a select with “select…. Where .. id in ();”
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-212001526
No problem, I appreciate your comments and think they are really helpful ;)
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211962282
@nvazquez I have added this to my CI queue. I have a pretty serious backlog, so it may take me a few days to get to it though. In the mean time, lets see if we can get some LGTM code reviews. :)
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-212097103
@Nv, that is great. The code now LGTM.
There is a problem with Jenkins, maybe a push –f can solve it?
Your work as always is very good ;)
I believe now we would need to run the integration 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218007214
@nvazquez: strange, this did not run any tests. Any ideas on that one?
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced smoke/test_list_ids_parameter.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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r62102918
--- Diff: test/integration/smoke/test_list_ids_parameter.py ---
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+""" Tests for API listing methods using 'ids' parameter
+"""
+#Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import (cleanup_resources,
+ validateList)
+from marvin.lib.base import (Account,
+ Volume,
+ DiskOffering)
+from marvin.lib.common import (get_domain,
+ get_zone)
+from marvin.codes import FAILED, PASS
+from nose.plugins.attrib import attr
+#Import System modules
+import time
+
+_multiprocess_shared_ = True
+class TestListIdsParams(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ testClient = super(TestListIdsParams, cls).getClsTestClient()
+ cls.apiclient = testClient.getApiClient()
+ cls.services = testClient.getParsedTestDataConfig()
+ cls.hypervisor = testClient.getHypervisorInfo()
+
+ cls.domain = get_domain(cls.apiclient)
+ cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests())
+
+ cls.disk_offering = DiskOffering.create(
+ cls.apiclient,
+ cls.services["disk_offering"]
+ )
+
+ cls.account = Account.create(
+ cls.apiclient,
+ cls.services["account"],
+ domainid=cls.domain.id
+ )
+
+ cls.volumes = []
+ testVolumes = {0: {'diskname': 'TestDiskServ'}, 1: {'diskname': 'TestDiskServ2'}, 2: {'diskname': 'TestDiskServ3'}}
+
+ for k, v in testVolumes.items():
+ volume = Volume.create(
+ cls.apiclient,
+ v,
+ zoneid=cls.zone.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid,
+ diskofferingid=cls.disk_offering.id
+ )
+ cls.debug("Created a volume with ID: %s" % volume.id)
+ cls.volumes.append(volume)
+
+ cls._cleanup = [
+ cls.disk_offering,
+ cls.account
+ #,cls.volumes[0],
--- End diff --
@nvazquez ,
Sorry to let you down. I am terrible with python.
I believe @rhtyd, @swill, or @DaanHoogland can help you with 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218464050
@nvazquez this seems to have broken travis for everyone. We have this issue showing up in pretty much all travis runs right now `ContextSuite context=TestListIdsParams>:teardown`.
Can you investigate? 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-210566503
Hi @rafaelweingartner, yes, I did test them. I provided new parameters in API calls and they retrieved objects as expected.
About the implementation, I was aware that <code>listVirtualMachines</code> had this parameter already implemented, so tried replicating the same idea, as it is in <code>QueryManagerImpl</code> in lines 792, 847, 937
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211487161
Thanks @rafaelweingartner I made a refactor based in your comments. I could remove "extends ManagerBase" from <code>QueryManagerImpl</code> but not from <code>VMSnapshotManagerImpl</code> and <code>SnapshotManagerImpl</code> so I decided to create static methods and use the new class as a helper instead of a superclass, do you think this is ok?
I couldn't find <code>@Local</code> to remove it, in which file did you mean to remove 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-210569587
Yes, it is only this proposal. Cool, I'll squash them
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211499145
@nv I am sorry, I might have seen the “@Local” annotation in some other PR, and I thought I have seen on yours.
The classes VMSnapshotManagerImpl and SnapshotManagerImpl are using something from “ManagerBase”?
I am ok with the static methods if there is no way to create a class hierarchic. They are clean and well documented. I am only missing the test cases for those two new methods.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211581957
That is it, and then this new class would extend ManagerBase.
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-217880689
Hi @koushik-das thanks for reviewing! Actually it took so long due to <code>setUpClass</code> method, after that, test are really simple and quick. I included 12 minutes sleep as you noticed due to vm snapshots bug, remaining time was (vm, templates, snapshots and vmsnapshots) creation
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218048949
No problem ;)
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218467920
@swill it's strange, as @serg38 says it succeeds for us and for @koushik-das, I'll investigate. Is there any log of the failure?
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211957155
@rafaelweingartner I pushed new changes, would integration tests be needed for this pr? I have tested api calls with new ids parameters and succeeded
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218005270
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 0
Errors: 0
Duration: 4h 38m 35s
```
**Associated Uploads**
**`/tmp/MarvinLogs/DeployDataCenter__May_09_2016_07_17_28_RFHUY6:`**
* [dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/DeployDataCenter__May_09_2016_07_17_28_RFHUY6/dc_entries.obj)
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/DeployDataCenter__May_09_2016_07_17_28_RFHUY6/failed_plus_exceptions.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/DeployDataCenter__May_09_2016_07_17_28_RFHUY6/runinfo.txt)
**`/tmp/MarvinLogs/test_network_DL9JLX:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_network_DL9JLX/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_network_DL9JLX/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_network_DL9JLX/runinfo.txt)
**`/tmp/MarvinLogs/test_suite_9BKAZW:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_suite_9BKAZW/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_suite_9BKAZW/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_suite_9BKAZW/runinfo.txt)
**`/tmp/MarvinLogs/test_vpc_routers_MX5M6P:`**
* [failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_vpc_routers_MX5M6P/failed_plus_exceptions.txt)
* [results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_vpc_routers_MX5M6P/results.txt)
* [runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1497/tmp/MarvinLogs/test_vpc_routers_MX5M6P/runinfo.txt)
Uploads will be available until `2016-07-09 02:00:00 +0200 CEST`
*Comment created by [`upr comment`](https://github.com/cloudops/upr).*
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218321496
Done, now it passed but somehow Jenkins test was not executed
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r59923092
--- Diff: server/src/com/cloud/api/query/QueryManagerImpl.java ---
@@ -1731,6 +1731,17 @@
Long zoneId = cmd.getZoneId();
Long podId = cmd.getPodId();
+ List<Long> ids = null;
--- End diff --
Ok, they extend from <code>com.cloud.utils.component.ManagerBase</code> but I found an issue on that, from its project (cloud-utils) there is no visibility to <code>com.cloud.exception.InvalidParameterValueException</code>
I wouldn't like to modify dependencies, what do you think? Would I use another exception?
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r62095734
--- Diff: test/integration/smoke/test_list_ids_parameter.py ---
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+""" Tests for API listing methods using 'ids' parameter
+"""
+#Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import (cleanup_resources,
+ validateList)
+from marvin.lib.base import (Account,
+ Volume,
+ DiskOffering)
+from marvin.lib.common import (get_domain,
+ get_zone)
+from marvin.codes import FAILED, PASS
+from nose.plugins.attrib import attr
+#Import System modules
+import time
+
+_multiprocess_shared_ = True
+class TestListIdsParams(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ testClient = super(TestListIdsParams, cls).getClsTestClient()
+ cls.apiclient = testClient.getApiClient()
+ cls.services = testClient.getParsedTestDataConfig()
+ cls.hypervisor = testClient.getHypervisorInfo()
+
+ cls.domain = get_domain(cls.apiclient)
+ cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests())
+
+ cls.disk_offering = DiskOffering.create(
+ cls.apiclient,
+ cls.services["disk_offering"]
+ )
+
+ cls.account = Account.create(
+ cls.apiclient,
+ cls.services["account"],
+ domainid=cls.domain.id
+ )
+
+ cls.volumes = []
+ testVolumes = {0: {'diskname': 'TestDiskServ'}, 1: {'diskname': 'TestDiskServ2'}, 2: {'diskname': 'TestDiskServ3'}}
+
+ for k, v in testVolumes.items():
+ volume = Volume.create(
+ cls.apiclient,
+ v,
+ zoneid=cls.zone.id,
+ account=cls.account.name,
+ domainid=cls.account.domainid,
+ diskofferingid=cls.disk_offering.id
+ )
+ cls.debug("Created a volume with ID: %s" % volume.id)
+ cls.volumes.append(volume)
+
+ cls._cleanup = [
+ cls.disk_offering,
+ cls.account
+ #,cls.volumes[0],
--- End diff --
I pushed this commented lines to ask about this, if I add them test fails but I don't understand how they are deleted. After test finishes I see volumes are deleted
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r60256696
--- Diff: server/test/com/cloud/api/query/MutualExclusiveIdsManagerBaseTest.java ---
@@ -0,0 +1,84 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+package com.cloud.api.query;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import com.cloud.utils.db.SearchCriteria;
+
+@RunWith(MockitoJUnitRunner.class)
+public class MutualExclusiveIdsManagerBaseTest {
+
+ @Mock
+ SearchCriteria<String> sc;
+
+ private static Long id1 = Long.valueOf(1l);
+ private static Long id2 = Long.valueOf(2l);
+
+ private List<Long> idsList;
+ private List<Long> idsEmptyList;
+ private List<Long> expectedListId;
+ private List<Long> expectedListIds;
+
+ private MutualExclusiveIdsManagerBase mgr = new MutualExclusiveIdsManagerBase();
+
+ @Before
+ public void setup() {
+ idsList = Arrays.asList(id1, id2);
+ idsEmptyList = Arrays.asList();
+ expectedListId = Arrays.asList(id1);
+ expectedListIds = Arrays.asList(id1, id2);
+ }
+
+ @Test
+ public void testSetIdsListToSearchCriteria(){
+ mgr.setIdsListToSearchCriteria(sc, idsList);
+ Mockito.verify(sc, times(1)).setParameters(Mockito.same("idIN"), Mockito.same(id1), Mockito.same(id2));
+ }
+
+ @Test
+ public void testSetIdsListToSearchCriteriaEmptyList(){
+ mgr.setIdsListToSearchCriteria(sc, idsEmptyList);
+ Mockito.verify(sc, never()).setParameters(Mockito.anyString(), Mockito.any());
+ }
+
+ @Test
+ public void testGetIdsListId(){
+ List<Long> result = mgr.getIdsListFromCmd(id1, idsEmptyList);
+ assertEquals(result, expectedListId);
--- End diff --
This is more a question of philosophy, but the assertEquals should be: assertEquals(expected, actual). You are doing the opposite. It does not mean that the way it is coded now is wrong, it is just not the normal way (at least that I am 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218055176
Sorry, apparently I am not getting enough sleep now days. I looked at your code after you updated it to add the tags, which is why I was confused why the tests were not run in my case. I thought I had checked that they had tags when I added them to my tests, but I blame on that on being tired too. :)
This one is ready to merge. Thanks guys...
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-212265582
@nvazquez Can you add integration tests for all these APIs? Checkout test_deploy_vm_multiple test in test/integration/smoke/test_vm_life_cycle.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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-210567690
Ah ok, so our home made JPA takes care of that ;)
That felt a little weird to me, but now I understand why.
I would only suggest you squashing your commits into one. It is a single proposal right?
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211993545
@rafaelweingartner Actually the idea was that they were mutually exclusive, that only of them should be provided, emulating `listVirtualMachines` API method behaviour in which if both are not null, it fails.
In general cases I think a good idea show be merging them, but in this case I think it should emulate this behaviour, maybe renaming the method to a more descriptive name?
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-217789291
@nvazquez The test results show that it took ~20mins to complete. Why list tests are taking so long?
Some observations:
- Tests are not tagged, please tag them (refer to existing tests). I ran them in a simulator setup and they passed so these can be added to Travis once the overall time is reduced and proper tags added
- Templates created as part of tests is not cleaned up.
- time.sleep(720). I saw the comment about vm snapshot issue, better to disable the vm snapshot test and enable it once snapshot issue is fixed
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r60255794
--- Diff: server/test/com/cloud/api/query/MutualExclusiveIdsManagerBaseTest.java ---
@@ -0,0 +1,84 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+package com.cloud.api.query;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import com.cloud.utils.db.SearchCriteria;
+
+@RunWith(MockitoJUnitRunner.class)
+public class MutualExclusiveIdsManagerBaseTest {
+
+ @Mock
+ SearchCriteria<String> sc;
+
+ private static Long id1 = Long.valueOf(1l);
+ private static Long id2 = Long.valueOf(2l);
+
+ private List<Long> idsList;
+ private List<Long> idsEmptyList;
+ private List<Long> expectedListId;
+ private List<Long> expectedListIds;
+
+ private MutualExclusiveIdsManagerBase mgr = new MutualExclusiveIdsManagerBase();
+
+ @Before
+ public void setup() {
+ idsList = Arrays.asList(id1, id2);
+ idsEmptyList = Arrays.asList();
+ expectedListId = Arrays.asList(id1);
+ expectedListIds = Arrays.asList(id1, id2);
+ }
+
+ @Test
+ public void testSetIdsListToSearchCriteria(){
+ mgr.setIdsListToSearchCriteria(sc, idsList);
+ Mockito.verify(sc, times(1)).setParameters(Mockito.same("idIN"), Mockito.same(id1), Mockito.same(id2));
--- End diff --
the default of Mockito.verify already uses "times(1)"; so, there is no need to inform 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1497#discussion_r62456142
--- Diff: test/integration/smoke/test_list_ids_parameter.py ---
@@ -0,0 +1,293 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+""" Tests for API listing methods using 'ids' parameter
+"""
+#Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import (cleanup_resources,
+ validateList)
+from marvin.lib.base import (Account,
+ Volume,
+ DiskOffering,
+ Template,
+ ServiceOffering,
+ Snapshot,
+ VmSnapshot,
+ VirtualMachine)
+from marvin.lib.common import (get_domain,
+ get_zone, get_template)
+from marvin.codes import FAILED, PASS
+from nose.plugins.attrib import attr
+#Import System modules
+import time
+
+_multiprocess_shared_ = True
+class TestListIdsParams(cloudstackTestCase):
+
+ @classmethod
+ def setUpClass(cls):
+ testClient = super(TestListIdsParams, cls).getClsTestClient()
+ cls.apiclient = testClient.getApiClient()
+ cls.services = testClient.getParsedTestDataConfig()
+ cls.hypervisor = testClient.getHypervisorInfo()
+
+ cls.domain = get_domain(cls.apiclient)
+ cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests())
+
+ cls.disk_offering = DiskOffering.create(
+ cls.apiclient,
+ cls.services["disk_offering"]
+ )
+
+ cls.account = Account.create(
+ cls.apiclient,
+ cls.services["account"],
+ domainid=cls.domain.id
+ )
+ cls.service_offering = ServiceOffering.create(
+ cls.apiclient,
+ cls.services["service_offerings"]["tiny"]
+ )
+
+ template = get_template(
+ cls.apiclient,
+ cls.zone.id,
+ cls.services["ostype"]
+ )
+ if template == FAILED:
+ assert False, "get_template() failed to return template with description %s" % cls.services["ostype"]
+
+ cls.services["template"]["ostypeid"] = template.ostypeid
+ cls.services["template_2"]["ostypeid"] = template.ostypeid
+ cls.services["ostypeid"] = template.ostypeid
+ cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+ cls.services["mode"] = cls.zone.networktype
+
+ #Create 3 VMs
+ cls.virtual_machine_1 = VirtualMachine.create(
+ cls.apiclient,
+ cls.services["virtual_machine"],
+ templateid=template.id,
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+ serviceofferingid=cls.service_offering.id,
+ mode=cls.services["mode"]
+ )
+ cls.virtual_machine_2 = VirtualMachine.create(
+ cls.apiclient,
+ cls.services["virtual_machine"],
+ templateid=template.id,
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+ serviceofferingid=cls.service_offering.id,
+ mode=cls.services["mode"]
+ )
+ cls.virtual_machine_3 = VirtualMachine.create(
+ cls.apiclient,
+ cls.services["virtual_machine"],
+ templateid=template.id,
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+ serviceofferingid=cls.service_offering.id,
+ mode=cls.services["mode"]
+ )
+
+ #Sleep for 12 minutes after vms creation because of bug while taking snapshots.
+ time.sleep(720)
--- End diff --
Please remove sleep and disable vm snapshot tests for 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218031267
Sure @swill I pushed again. I also ran again tests in my environment:
````
[root@ussarlabcsmgt41 cloudstack]# nosetests --with-marvin --marvin-config=setup/dev/advanced.cfg test/integration/smoke/test_list_ids_parameter.py
==== Marvin Init Started ====
=== Marvin Parse Config Successful ===
=== Marvin Setting TestData Successful===
==== Log Folder Path: /tmp//MarvinLogs//May_09_2016_17_39_25_ETC1VH. All logs will be available here ====
=== Marvin Init Logging Successful===
==== Marvin Init Successful ====
===final results are now copied to: /tmp//MarvinLogs/test_list_ids_parameter_A7VS1H===
[root@ussarlabcsmgt41 cloudstack]# cat /tmp//MarvinLogs/test_list_ids_parameter_A7VS1H/results.txt
Test listing Volumes using 'ids' parameter ... === TestName: test_01_list_volumes | Status : SUCCESS ===
ok
Test listing Templates using 'ids' parameter ... === TestName: test_02_list_templates | Status : SUCCESS ===
ok
Test listing Snapshots using 'ids' parameter ... === TestName: test_03_list_snapshots | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 3 tests in 446.512s
OK
````
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218265436
Thanks @koushik-das @swill for testing! I'll push -f to restart Travis test which failed
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218469220
@serg38 I had a bunch of people re-push PRs last night and this morning to get everything green and 3 or 4 are failing with this.
Here are a couple off the top of my head: #1502, #1376
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218051058
LGTM, ran the new tests.
Test listing Volumes using 'ids' parameter ... === TestName: test_01_list_volumes | Status : SUCCESS ===
ok
Test listing Templates using 'ids' parameter ... === TestName: test_02_list_templates | Status : SUCCESS ===
ok
Test listing Snapshots using 'ids' parameter ... === TestName: test_03_list_snapshots | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 3 tests in 148.845s
OK
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218048625
@nvazquez My bad, didn't look at the changes. Thanks for the fixing the 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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-218500189
Thank you, I will merge it assuming Jenkins and Travis come back clean. Thx... \U0001f44d
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by nvazquez <gi...@git.apache.org>.
Github user nvazquez commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211540538
Cool, I'll working on test cases.
`VMSnapshotManagerImpl` is using `_name` variable from `ManagerBase,` and `SnapshotManagerImpl` overrides `configure`, `start` and `stop` method
---
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: CLOUDSTACK-9351: Add ids parameter to res...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1497#issuecomment-211988788
@nvazquez I have a question about the method "getIdsListFromCmd".
If we inform an ID and a list of IDs (not empty) at the same time, what should be the behaviors? Should we merge them in that list of IDs? Should we only use the ID? Or should we only use the IDs list?
---
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.
---