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... ð
---
If your project is set up for it, you can reply to this email and have your
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
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. ð
---
If your project is set up for it, you can repl
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_off
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
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=TestListIdsPara
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
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: tes
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
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 GitHu
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 hav
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
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 listin
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 fe
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
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 you
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 t
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
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=s
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
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 e
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
```
**Associ
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
setUpClass method, after that, test are really simple and quick. I
included 12 minutes sleep
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
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
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 ta
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-m
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
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
+
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 rep
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 you
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 p
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 b
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
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 no
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
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 a
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,
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 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
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 behavi
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
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 So
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 So
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 So
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 n
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
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
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
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
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 projec
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 extendin
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`
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 VMSnapshotManagerI
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 QueryManagerImpl but not from
VMSnapshotManagerImpl a
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
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 p
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);
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();
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 wel
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
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 awar
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 ma
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 exc
64 matches
Mail list logo
