Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-221940475
### ACS CI BVT Run
**Sumarry:**
Build Number 58
Hypervisor xenserver
NetworkType Advanced
Passed=67
Failed=4
Skipped=3
_
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1360
---
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 swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-220656774
Ran again because the error was not something I had seen often. The errors
in the new run are not related to this PR.
---
If your project is set up for it, you can
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-220656462
### CI RESULTS
```
Tests Run: 82
Skipped: 0
Failed: 1
Errors: 4
Duration: 10h 22m 36s
```
**Summary of the
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-219922258
I think this one is ready. The CI is coming back pretty clean and I have
the required code review.
---
If your project is set up for it, you can reply to this email
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-219920616
### CI RESULTS
```
Tests Run: 85
Skipped: 0
Failed: 0
Errors: 1
Duration: 4h 19m 39s
```
**Summary of the p
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r58875609
--- Diff: server/test/com/cloud/consoleproxy/ConsoleProxyManagerTest.java
---
@@ -87,4 +114,136 @@ public void testExisingCPVMStartFailure() throws
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r58874658
--- Diff:
services/secondary-storage/controller/src/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
---
@@ -521,6 +522,76 @@
Github user ProjectMoon commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r58852188
--- Diff: server/test/com/cloud/consoleproxy/ConsoleProxyManagerTest.java
---
@@ -87,4 +114,136 @@ public void testExisingCPVMStartFailure() throws
Github user ProjectMoon commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r58852087
--- Diff:
services/secondary-storage/controller/src/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
---
@@ -521,6 +522,76 @@
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r58672156
--- Diff:
services/secondary-storage/controller/src/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
---
@@ -521,6 +522,76 @@
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r58671820
--- Diff: server/test/com/cloud/consoleproxy/ConsoleProxyManagerTest.java
---
@@ -87,4 +114,136 @@ public void testExisingCPVMStartFailure() throws
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-199642025
### ACS CI BVT Run
**Sumarry:**
Build Number 117
Hypervisor xenserver
NetworkType Advanced
Passed=104
Failed=14
Skipped=4
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-185645919
PR rebased against latest master.
---
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 projec
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-179180819
I now consider the pull request ready to be merged. The unit test commit
has been split out and merged into the two commits for the console proxy and
SSVM chang
Github user rodrigo93 commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-178002003
@GabrielBrascher @ProjectMoon, You are right. I thought I saw a some
section in Jira for minor improvements, but I was wrong. Besides that, the code
and work done
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-177962933
I would prefer just leaving one pull request here, but with two commits.
Each commit would have the file changes + unit tests for the SSVM and CPVM,
respectivel
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-177942109
@rodrigo93 I think that a JIRA ticket wouldn't help much, considering that
JIRA is for report issues (not PRs). I think that the point here is to analyze
th
Github user rodrigo93 commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-177348201
Hi @ProjectMoon,
What a great work you did here.
Splitting the PR in two seems a good idea, in my point of view, since you
create two tests cases for dist
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-176671839
Should the three commits be squashed? Or left in three? I suggest perhaps
splitting the unit test commit into two, and putting each unit test with its
correspon
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-175751878
@ProjectMoon The code is better now, well documented and with tests.
LGTM.
---
If your project is set up for it, you can reply to this email and have yo
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-175727599
License has been added.
---
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
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-175639158
@ProjectMoon Yes, just need to add the license in
"SecondaryStorageManagerTest" class.
---
If your project is set up for it, you can reply to this email an
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-175634193
Seems the Jenkins build is complaining about license missing. Don't see
what file it is, but I assume it's the new unit test file?
---
If your project is set u
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-175039929
I have created the unit tests. They are in one commit. The tests test the
protected methods delegated to by getDefaultNetworkForCreation. It tests all 4
possibl
Github user pedro-martins commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-174571489
There isn't a test for this class if you want a suggestion, create a new
folder named test in cloud-controller-secondary-storage project and create a
new pack
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-174561850
Also, where is the best place to put tests for the secondary storage
manager? Is there already a test for it?
---
If your project is set up for it, you can rep
Github user pedro-martins commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-174539020
Yes, your import is correct, I've done a mistake, the collections4 (new
collection version) isn't used by ACS, sry for that ^_^'
---
If your project is set u
Github user ProjectMoon commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r50702050
--- Diff:
services/secondary-storage/controller/src/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
---
@@ -516,6 +517,79 @@
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r50699965
--- Diff:
services/secondary-storage/controller/src/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
---
@@ -516,6 +517,7
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-174517194
I've added the changes requested, minus the tests (still working on those).
I am pushing the changes now for review. @pedro-martins The CollectionUtils
library
Github user GabrielBrascher commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-174212703
@ProjectMoon Could you please do the following changes (1, 2, 3 and 4)?
**1** - create a method for the code between lines 672 and 676;
**2** - c
Github user GabrielBrascher commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1360#discussion_r50623870
--- Diff: server/src/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java ---
@@ -666,18 +666,10 @@ public ConsoleProxyVO startNew(long dataCenterI
Github user pedro-martins commented on the pull request:
https://github.com/apache/cloudstack/pull/1360#issuecomment-174210179
Could you replace the logic in your 'if' at lines 673 and 523 "(networks ==
null || networks.size() == 0)"
to CollectionUtils.isEmpty(networks) from
org.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1359#issuecomment-173872832
@ProjectMoon Indeed! :-)
---
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 no
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/1360
Refactor system VM default network creation
Two small commits which moves the retrieval of the default network for the
console proxy and the SSVM into a separate protected method. It's a sm
Github user ProjectMoon closed the pull request at:
https://github.com/apache/cloudstack/pull/1359
---
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 featu
Github user ProjectMoon commented on the pull request:
https://github.com/apache/cloudstack/pull/1359#issuecomment-173872098
OK. So just to be sure: it's oldest supported maintenance release for bug
fixes and then master for any new features/refactoring?
---
If your project is set up
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1359#issuecomment-173869657
Hi @ProjectMoon
Could you please create you PR against Master. If that's a refactor does
not really make sense to create the PR against 4.6 because
GitHub user ProjectMoon opened a pull request:
https://github.com/apache/cloudstack/pull/1359
Refactor system VM default network creation
Two small commits which moves the retrieval of the default network for the
console proxy and the SSVM into a separate protected method. It's a sm
40 matches
Mail list logo
