[GitHub] cloudstack issue #1269: CLOUDSTACK-8867: Added retry logic to reconnect to h...

2017-03-15 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1269 Thanks for the response @anshul1886. I am sorry, but I disagree with you. I think that it gets more stable and readable with less duplicated code. In my humble opinion

[GitHub] cloudstack issue #1989: WIP: support for multidisk OVA files

2017-03-09 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1989 Unless I missed something and this PR does not address the same issue at CLOUDSTACK-4757. In this case, I am sorry. --- If your project is set up for it, you can reply to this email

[GitHub] cloudstack issue #1989: WIP: support for multidisk OVA files

2017-03-09 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1989 @abhinandanprateek thanks for your work. Could you please append the Jira issue CLOUDSTACK-4757 with the PR title (https://issues.apache.org/jira/browse/CLOUDSTACK-4757

[GitHub] cloudstack issue #1956: CLOUDSTACK-9796 - Fix NPE in VirtualMachineManagerIm...

2017-03-09 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1956 Got it @borisstoyanov. 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

[GitHub] cloudstack issue #1956: CLOUDSTACK-9796 - Fix NPE in VirtualMachineManagerIm...

2017-03-09 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1956 Great, this one looks ready to merge. Just for caution. Are those failures false positives? ``` test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure

[GitHub] cloudstack issue #1278: CLOUDSTACK-9198: Virtual router gets deployed in dis...

2017-03-03 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1278 @anshul1886 I would like to raise the point previously discussed by me and @rafaelweingartner. I think that we should pay attention if the change of user and caller will really do

[GitHub] cloudstack pull request #1941: CLOUDSTACK-8663: Fixed various issues to allo...

2017-02-23 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1941#discussion_r102730127 --- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java --- @@ -1415,6 +1417,10 @@ public

[GitHub] cloudstack issue #1941: CLOUDSTACK-8663: Fixed various issues to allow VM sn...

2017-02-23 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1941 Thanks @anshul1886. I see that you improved this PR considering the comments from the one closed. Some tests failed. Are they false positives? Otherwise LGTM. Just need to confirm those

[GitHub] cloudstack pull request #1941: CLOUDSTACK-8663: Fixed various issues to allo...

2017-02-23 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1941#discussion_r102715520 --- Diff: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java --- @@ -1415,6 +1417,10 @@ public

[GitHub] cloudstack issue #1956: CLOUDSTACK-9796 - Fix NPE in VirtualMachineManagerIm...

2017-02-20 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1956 Thanks for the code that prevents NULL pointer exception. Based on code revirew and that all checks have passed LGTM. --- If your project is set up for it, you can reply

[GitHub] cloudstack pull request #1559: CLOUDSTACK-9280: System VM volumes can be exp...

2016-10-17 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1559#discussion_r83595364 --- Diff: engine/storage/src/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java --- @@ -345,15 +354,34 @@ public EndPoint

[GitHub] cloudstack pull request #1559: CLOUDSTACK-9280: System VM volumes can be exp...

2016-10-17 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1559#discussion_r83628876 --- Diff: engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeDataFactoryImpl.java --- @@ -77,22 +80,51 @@ public VolumeInfo

[GitHub] cloudstack pull request #1559: CLOUDSTACK-9280: System VM volumes can be exp...

2016-10-17 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1559#discussion_r83647727 --- Diff: engine/storage/test/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java --- @@ -0,0 +1,167 @@ +// Licensed

[GitHub] cloudstack pull request #1559: CLOUDSTACK-9280: System VM volumes can be exp...

2016-10-17 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1559#discussion_r83600411 --- Diff: engine/storage/test/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelectorTest.java --- @@ -0,0 +1,167 @@ +// Licensed

[GitHub] cloudstack issue #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Make NFS ...

2016-08-17 Thread GabrielBrascher
Github user GabrielBrascher commented on the issue: https://github.com/apache/cloudstack/pull/1615 @serg38 @nvazquez the code seems ok. Is there anything pending with the _blueorangutan_ test? --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] cloudstack pull request: Remodeling of Nuage VSP Plugin + CLOUDSTA...

2016-05-24 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1494#issuecomment-221349421 Thanks @nlivens, great work cleaning. The code LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] cloudstack pull request: Remove unused code at "com.cloud.vm.UserV...

2016-05-22 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1558 Remove unused code at "com.cloud.vm.UserVmManagerImpl" # Summary All changes in this PR started by removing unused methods from the "UserVmManagerImpl". That

[GitHub] cloudstack pull request: Remodeling of Nuage VSP Plugin + CLOUDSTA...

2016-05-20 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1494#discussion_r64080090 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspEntityBuilder.java --- @@ -0,0 +1,370 @@ +// +// Licensed

[GitHub] cloudstack pull request: Remodeling of Nuage VSP Plugin + CLOUDSTA...

2016-05-20 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1494#discussion_r64078451 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResource.java --- @@ -270,15 +258,15 @@ public PingCommand

[GitHub] cloudstack pull request: CLOUDSTACK-6928: fix issue disk I/O throt...

2016-05-19 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1410#issuecomment-220382990 Thanks for the update @ustcweizhou, now I can't find anything to :-1: this code. Based on code review and the CI result, the code LGTM. --- If your

[GitHub] cloudstack pull request: CLOUDSTACK-9380: fix NPE in listDomains A...

2016-05-17 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1550#issuecomment-219772135 Based on code review, LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project

[GitHub] cloudstack pull request: Reimplement router.redundant.vrrp.interva...

2016-05-16 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1486#issuecomment-219561259 Based on code review and the documentation cited by @remibergsma, the code LGTM. --- If your project is set up for it, you can reply to this email

[GitHub] cloudstack pull request: CLOUDSTACK-9377: Fix metrics pagesize iss...

2016-05-16 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1540#issuecomment-219430432 The idea and screenshot seem nice, the code is ok. LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] cloudstack pull request: CLOUDSTACK-9368: Fix for Support configur...

2016-05-14 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1518#issuecomment-219249718 @nvazquez sorry for the late review, I could point just 2 typos on Javadoc and some variables with the underscore (would be nice to change those

[GitHub] cloudstack pull request: CLOUDSTACK-9368: Fix for Support configur...

2016-05-14 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1518#discussion_r63281291 --- Diff: plugins/hypervisors/vmware/src/com/cloud/storage/resource/VmwareStorageSubsystemCommandHandler.java --- @@ -61,11 +61,28 @@ public

[GitHub] cloudstack pull request: CLOUDSTACK-9368: Fix for Support configur...

2016-05-14 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1518#discussion_r63281246 --- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java --- @@ -303,6 +305,7 @@ protected

[GitHub] cloudstack pull request: CLOUDSTACK-9368: Fix for Support configur...

2016-05-14 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1518#discussion_r63281205 --- Diff: plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java --- @@ -519,6 +523,64 @@ public Answer

[GitHub] cloudstack pull request: CLOUDSTACK-6928: fix issue disk I/O throt...

2016-05-12 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1410#issuecomment-218869697 I don't see anything wrong with this PR. However, the point made by @rodrigo93 is interesting. At the class "VolumeOrchestrator" (lines

[GitHub] cloudstack pull request: CLOUDSTACK-6928: fix issue disk I/O throt...

2016-05-12 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1410#issuecomment-218850613 @DaanHoogland @ustcweizhou I noticed that there is four 'if' with the condition `(io != null) && (io > 0)` at the _KVMStorageProcessor_ class (

[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks

2016-05-03 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1365#issuecomment-216571886 @swill I haven't worked much on that file (I had fixed a typo in the name of the "ROOT_DISK_CONTROLLER" variable). At the first look I w

[GitHub] cloudstack pull request: CLOUDSTACK-9289:Automation for feature de...

2016-04-25 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1417#issuecomment-214337105 Based on the code review, LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] cloudstack pull request: Bug-ID: CLOUDSTACK-8870: Skip external de...

2016-04-21 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/846#issuecomment-212999101 Although I had pointed a typo in a comment line (nothing serious), the code LGTM. --- If your project is set up for it, you can reply to this email

[GitHub] cloudstack pull request: CLOUDSTACK-9352: Test fails in Widows as ...

2016-04-21 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1498#issuecomment-212984070 @swill it succesfully builds in windows: [build-result.txt](https://github.com/apache/cloudstack/files/230237/build-result.txt); Not sure

[GitHub] cloudstack pull request: CLOUDSTACK-9261: Query to traffic sentine...

2016-04-21 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1418#issuecomment-212876630 @kansal create methods would also help to test the behavior of this code, using unit test and integration test (to verify if the methods that have unit

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-20 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-212512170 @DaanHoogland I removed `@param`, `@return` and `@throws` from javadoc. Thanks. --- If your project is set up for it, you can reply to this email and have

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-20 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1459#discussion_r60444764 --- Diff: utils/src/main/java/com/cloud/utils/ssh/SshHelper.java --- @@ -206,4 +216,87 @@ public static void scpTo(String host, int port, String

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-19 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-211989957 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

[GitHub] cloudstack pull request: Test fails in Widows as the file separato...

2016-04-16 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1498 Test fails in Widows as the file separator "/" is different from "\" **Problem:** File separator in windows ("\") is different from the expected in t

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...

2016-04-15 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1493#discussion_r59894203 --- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java --- @@ -19,146 +19,198 @@ package com.cloud.utils.testcase

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...

2016-04-15 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1493#discussion_r59892756 --- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java --- @@ -19,146 +19,198 @@ package com.cloud.utils.testcase

[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...

2016-04-14 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1493#discussion_r59790778 --- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java --- @@ -19,146 +19,198 @@ package com.cloud.utils.testcase

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-12 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-209084883 I am sorry, but I forgot to update the com.cloud.api.ApiServletTest class, now that those tests are running, all checks have passed. @bhaisaab

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-12 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-208969926 @bhaisaab force pushed. --- 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

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-11 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-208533273 Travis keeps with the issue *missing file/directory '/home/travis/build/apache/cloudstack/tools/marvin/dist/Marvin-***.tar.gz*'. --- If your project

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-11 Thread GabrielBrascher
Github user GabrielBrascher closed the pull request at: https://github.com/apache/cloudstack/pull/1263 --- 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

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-11 Thread GabrielBrascher
GitHub user GabrielBrascher reopened a pull request: https://github.com/apache/cloudstack/pull/1263 Removed unused code from com.cloud.api.ApiServer **Removed “\_” from variables names**: private variables with “\_” at the beginning is common in C++ but not in Java

[GitHub] cloudstack pull request: Remove unused params from NetworkHelperIm...

2016-04-11 Thread GabrielBrascher
Github user GabrielBrascher closed the pull request at: https://github.com/apache/cloudstack/pull/1484 --- 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

[GitHub] cloudstack pull request: Remove unused params from NetworkHelperIm...

2016-04-11 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1484#issuecomment-208411575 Thanks for noting @pedro-martins. As you stated this issue before, I will close mine PR. Unless you think that another of proposed modifications

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-10 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-207989355 @DaanHoogland thanks for the tip. --- 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

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-10 Thread GabrielBrascher
GitHub user GabrielBrascher reopened a pull request: https://github.com/apache/cloudstack/pull/1263 Removed unused code from com.cloud.api.ApiServer **Removed “\_” from variables names**: private variables with “\_” at the beginning is common in C++ but not in Java

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-10 Thread GabrielBrascher
Github user GabrielBrascher closed the pull request at: https://github.com/apache/cloudstack/pull/1263 --- 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

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-09 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-207914050 Seems that `push -f` is not making the Jenkins and Travis to rerun their tests. Is there any workaround for that? --- If your project is set up

[GitHub] cloudstack pull request: Remove unused params from NetworkHelperIm...

2016-04-09 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1484 Remove unused params from NetworkHelperImpl.start method The contributions of this PR are: - Removed unused params ("User" and "Account") from:

[GitHub] cloudstack pull request: Remove unused images

2016-04-08 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1475#issuecomment-207570464 Remove unused files is always good. LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] cloudstack pull request: Cloudstack-9285 exception log addition

2016-04-08 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1479#issuecomment-207507877 Got it @kiwiflyer. I am good with the way it is. Simple logger change, the code LGTM. --- If your project is set up for it, you can reply to this email

[GitHub] cloudstack pull request: Cloudstack-9285 exception log addition

2016-04-08 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1479#issuecomment-207501906 @kiwiflyer shouldn't the `s_logger.warn("NIO Connection Exception " + e);` be after the info `s_logger.info("Attempted to connect&qu

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-08 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-207488773 @pdube removed those uderscores, also I modified the "com.cloud.api.ApiServer.setEncodeApiResponse(boolean)" method to static again. Thanks. -

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-04-08 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1263#discussion_r59043684 --- Diff: server/src/com/cloud/api/ApiServlet.java --- @@ -156,7 +158,7 @@ void processRequestInContext(final HttpServletRequest req, final

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-08 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-207446285 @rafaelweingartner now I am ensuring that the "Thread.sleep()" method is being called properly. Also I removed the method that was checking if th

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-07 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-207159361 @rafaelweingartner now I am using PowerMockito to test the "throwSshExceptionIfSshConnectionIsNull" method without the need to wait for the &qu

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-07 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-206931826 @swill thanks for the effort. Don't worry, I can wait ;) --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks

2016-04-06 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1365#discussion_r58781801 --- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java --- @@ -1924,6 +1935,23 @@ public Volume migrateVolume(MigrateVolumeCmd cmd

[GitHub] cloudstack pull request: New test to validate starting vm after ni...

2016-04-06 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1326#issuecomment-206391435 Based on the code LGTM. However, I am not sure if should consider those failed tests ("integration.smoke.test_guest_vlan_range.TestDedicateGuestVla

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-05 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-205932840 @rafaelweingartner now this commit is not cherry-picked from the commit (b9181c689e0e7b5f1e28c81d73710196dfabd0ba). When this PR be merged (or closed

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-05 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-205755861 Thanks @DaanHoogland, I will check on that. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-04 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1459#discussion_r58466286 --- Diff: utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java --- @@ -0,0 +1,131 @@ +// +// Licensed to the Apache Software

[GitHub] cloudstack pull request: CLOUDSTACK-8611:Handle SSH if server "for...

2016-04-04 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1459#discussion_r58442410 --- Diff: utils/src/test/java/com/cloud/utils/ssh/SshHelperTest.java --- @@ -0,0 +1,131 @@ +// +// Licensed to the Apache Software

[GitHub] cloudstack pull request: Cloudstack-8611:Handle SSH if server "for...

2016-04-01 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-204571209 Correcting myself, the history from the SshHelper (https://github.com/likitha/cloudstack/commits/CLOUDSTACK-8611/utils/src/com/cloud/utils/ssh

[GitHub] cloudstack pull request: Cloudstack-8611:Handle SSH if server "for...

2016-04-01 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-204484447 @DaanHoogland thanks for the hint, but the problem is not related to line ending. The problem is that there are two (2) different SshHelper classes

[GitHub] cloudstack pull request: Cloudstack 8611 Handle SSH if server "for...

2016-04-01 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1459#issuecomment-204293184 Not sure why it shows as if I had changed all the SshHelper class (the diff shows removed lines that I have not changed). I will check

[GitHub] cloudstack pull request: Cloudstack 8611 Handle SSH if server "for...

2016-04-01 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1459 Cloudstack 8611 Handle SSH if server "forget" to send exit status Continuing the work started by @likitha, I cherry-picked the commit (b9181c689e0e7b5f1e28c81d737101

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-03-31 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-203947763 @DaanHoogland @rafaelweingartner @pdube removed those ".class" files, also all checks have passed. Thanks. --- If your project is set up f

[GitHub] cloudstack pull request: Removed unused code from com.cloud.api.Ap...

2016-03-30 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1263#issuecomment-203492750 @DaanHoogland @rafaelweingartner well noted, I have to remove those ".class" files; @pdube I am going to verify this checkstyle error.

[GitHub] cloudstack pull request: Remove classes with no references

2016-03-30 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1453#issuecomment-203484371 Thank you @pdube! --- 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] cloudstack pull request: New test to validate starting vm after ni...

2016-03-27 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1326#discussion_r57545397 --- Diff: test/integration/component/test_add_remove_network.py --- @@ -1021,6 +1021,103 @@ def test_29_remove_nic_CS22503(self

[GitHub] cloudstack pull request: CLOUDSTACK-9315: Removed unused Classes

2016-03-21 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1448#issuecomment-199521289 @DaanHoogland @swill thank you for the concern. I agree with the need for executing integration/functional tests to keep the code stable. Unfortunately

[GitHub] cloudstack pull request: CLOUDSTACK-9315: Removed unused Classes

2016-03-21 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1448#issuecomment-199272015 @DaanHoogland the lack of test in some classes indeed is a problem. However, these classes that I removed are not being used. I used UCDetector

[GitHub] cloudstack pull request: Removed unused Classes

2016-03-20 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1448 Removed unused Classes Remove unused Classes (classes with no references: - com.cloud.agent.api.CheckStateAnswer - com.cloud.agent.api.StartupVMMAgentCommand

[GitHub] cloudstack pull request: Removed unused code from CloudZoneStartup...

2016-03-19 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1446#issuecomment-198803008 @eriweb, I didn't considered JIRA looking to this point of view (track changes in the release note). With that in mind it seems an interesting approach

[GitHub] cloudstack pull request: Removed unused code from CloudZoneStartup...

2016-03-19 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1446#issuecomment-198794064 @eriweb, as far as I know, JIRA is used for bug/issue tracking. JIRA is a way that users have to report bugs. Based on JIRA, developers can track and fix

[GitHub] cloudstack pull request: Remove unused code from XenServerStorageP...

2016-03-19 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1443 Remove unused code from XenServerStorageProcessor and change methods access level Remove unused code from "com.cloud.hypervisor.xenserver.resource.XenServerStorageProcessor&q

[GitHub] cloudstack-docs-admin pull request: OSPF: WIP formatting documenta...

2016-03-19 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack-docs-admin/pull/36#issuecomment-198702503 Hi @agneya2001, could you please consider the following suggestions? - Line 1453, change from "debian" to "Debian";

[GitHub] cloudstack pull request: removed unused HypervDummyResourceBase cl...

2016-03-11 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1437#issuecomment-195618530 Given that @pedro-martins had no problems on running tests, the code LGTM. --- If your project is set up for it, you can reply to this email and have your

[GitHub] cloudstack pull request: Change variable "ROOK_DISK_CONTROLLER" to...

2016-03-09 Thread GabrielBrascher
GitHub user GabrielBrascher opened a pull request: https://github.com/apache/cloudstack/pull/1434 Change variable "ROOK_DISK_CONTROLLER" to "ROOT_DISK_CONTROLLER" Change com.cloud.vm.VmDetailConstants variable name from "ROOK_DISK_CONTROLLER&

[GitHub] cloudstack pull request: CLOUDSTACK-7985: assignVM in Advanced zon...

2016-03-09 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/844#discussion_r55611837 --- Diff: client/WEB-INF/classes/resources/messages.properties --- @@ -1823,6 +1823,7 @@ message.after.enable.swift=Swift configured. Note\: When

[GitHub] cloudstack pull request: CLOUDSTACK-8611. CS waits indefinitely fo...

2016-03-07 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/561#issuecomment-193216137 That's fair enough @DaanHoogland, I will see what I can do for this PR. --- If your project is set up for it, you can reply to this email and have your

[GitHub] cloudstack pull request: CLOUDSTACK-9289:Automation for feature de...

2016-03-06 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1417#discussion_r55160508 --- Diff: test/integration/component/test_deploy_vm_from_snapshot.py --- @@ -0,0 +1,644 @@ +# Licensed to the Apache Software Foundation (ASF

[GitHub] cloudstack pull request: CLOUDSTACK-8611. CS waits indefinitely fo...

2016-03-06 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/561#issuecomment-193063329 Shouldn't the class throw a less generic Exception? A good choice might be the **com.cloud.utils.ssh.SshException.SshException(String)**. Although

[GitHub] cloudstack pull request: CLOUDSTACK-9289:Automation for feature de...

2016-03-06 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1417#discussion_r55153415 --- Diff: test/integration/component/test_deploy_vm_from_snapshot.py --- @@ -0,0 +1,644 @@ +# Licensed to the Apache Software Foundation (ASF

[GitHub] cloudstack pull request: CLOUDSTACK-9184: [VMware] vmware.ports.pe...

2016-03-06 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1253#issuecomment-192951641 @sureshanaparti Although it might be a bit tricky, I believe that the ideal solution would be to check for the hypervisor version; if it is 4.1/4.0

[GitHub] cloudstack pull request: CLOUDSTACK-9287 - Fix unique mac address ...

2016-03-05 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1413#discussion_r55127726 --- Diff: server/src/com/cloud/network/element/VpcVirtualRouterElement.java --- @@ -559,9 +559,16 @@ public boolean applyACLItemsToPrivateGw

[GitHub] cloudstack pull request: CLOUDSTACK-9287 - Fix unique mac address ...

2016-03-05 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1413#discussion_r55127619 --- Diff: server/src/com/cloud/network/element/VpcVirtualRouterElement.java --- @@ -559,9 +559,16 @@ public boolean applyACLItemsToPrivateGw

[GitHub] cloudstack pull request: ADD be explicit about the underlying limi...

2016-03-05 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1426#issuecomment-192720921 It seems a fair update. LGTM due to the lack of code. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] cloudstack pull request: CLOUDSTACK-9175: [VMware DRS] Adding new ...

2016-02-28 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1257#discussion_r54359804 --- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/HostMO.java --- @@ -1110,4 +1148,39 @@ public String getNetworkName(String netMorVal

[GitHub] cloudstack pull request: CLOUDSTACK-9261: Query to traffic sentine...

2016-02-28 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1418#issuecomment-189941771 @kansal Could you please consider the following adjustments? - create a method for the "Part 1" (lines 213-219). With the command as

[GitHub] cloudstack pull request: CLOUDSTACK-9252: Support configurable NFS...

2016-02-23 Thread GabrielBrascher
Github user GabrielBrascher commented on the pull request: https://github.com/apache/cloudstack/pull/1361#issuecomment-188040784 @nvazquez LGTM based on integration tests and code. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub

[GitHub] cloudstack pull request: CLOUDSTACK-8609. [VMware] VM is not acces...

2016-02-21 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/556#discussion_r53570662 --- Diff: vmware-base/src/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java --- @@ -880,6 +880,38 @@ else if (prop.getName().startsWith("

[GitHub] cloudstack pull request: CLOUDSTACK-8829 : Consecutive cold migrat...

2016-02-20 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/797#discussion_r53559299 --- Diff: engine/orchestration/src/com/cloud/vm/VirtualMachineManagerImpl.java --- @@ -1776,19 +1773,26 @@ private void

[GitHub] cloudstack pull request: CLOUDSTACK-6928: fix issue disk I/O throt...

2016-02-20 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1410#discussion_r53556330 --- Diff: plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java --- @@ -1006,6 +1006,19 @@ protected

[GitHub] cloudstack pull request: CLOUDSTACK-9280: Allow system VM volumes ...

2016-02-17 Thread GabrielBrascher
Github user GabrielBrascher commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1406#discussion_r53150413 --- Diff: engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java --- @@ -92,7 +92,7 @@ public static VolumeObject

  1   2   >