[GitHub] rhtyd closed pull request #2662: ui: Fixes #2558 use POST for uploadSslCert API request (#2661)

[GitBox]

rhtyd closed pull request #2662: ui: Fixes #2558 use POST for uploadSslCert API 
request (#2661)
URL: https://github.com/apache/cloudstack/pull/2662
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rhtyd closed issue #2558: API uploadSslCert does not work

[GitBox]

rhtyd closed issue #2558: API uploadSslCert does not work
URL: https://github.com/apache/cloudstack/issues/2558
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[cloudstack] 01/01: Merge apache/4.11

[rohit]

This is an automated email from the ASF dual-hosted git repository.

rohit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git

commit ea373eb1e5d73626427e9cb36248869613ba53e3
Merge: ada1e73 eb23d91
Author: Rohit Yadav 
AuthorDate: Wed May 23 09:36:03 2018 +0530

Merge apache/4.11

ui: Fixes #2558 use POST for uploadSslCert API request (#2661)

 ui/scripts/accounts.js | 1 +
 1 file changed, 1 insertion(+)


-- 
To stop receiving notification emails like this one, please contact
ro...@apache.org.

[cloudstack] branch master updated (ada1e73 -> ea373eb)

[rohit]

This is an automated email from the ASF dual-hosted git repository.

rohit pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git.


from ada1e73  Merge branch '4.11'
 add eb23d91  ui: Fixes #2558 use POST for uploadSslCert API request (#2661)
 new ea373eb  Merge apache/4.11

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 ui/scripts/accounts.js | 1 +
 1 file changed, 1 insertion(+)

-- 
To stop receiving notification emails like this one, please contact
ro...@apache.org.

[GitHub] rhtyd opened a new pull request #2662: ui: Fixes #2558 use POST for uploadSslCert API request (#2661)

[GitBox]

rhtyd opened a new pull request #2662: ui: Fixes #2558 use POST for 
uploadSslCert API request (#2661)
URL: https://github.com/apache/cloudstack/pull/2662
 
 
   ## Description
   
   
   
   
   
   ## Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing 
functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## GitHub Issue/PRs
   
   
   
   
   
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   
   
   
   
   
   ## Checklist:
   
   
   - [ ] I have read the 
[CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md)
 document.
   - [ ] My code follows the code style of this project.
   - [ ] My change requires a change to the documentation.
   - [ ] I have updated the documentation accordingly.
   Testing
   - [ ] I have added tests to cover my changes.
   - [ ] All relevant new and existing integration tests have passed.
   - [ ] A full integration testsuite with all test that can run on my 
environment has passed.
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rhtyd closed pull request #2661: Make uploadSslCert a POST request instead of a GET

[GitBox]

rhtyd closed pull request #2661: Make uploadSslCert a POST request instead of a 
GET
URL: https://github.com/apache/cloudstack/pull/2661
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/ui/scripts/accounts.js b/ui/scripts/accounts.js
index a9f7381e641..192c284ebd5 100644
--- a/ui/scripts/accounts.js
+++ b/ui/scripts/accounts.js
@@ -1015,6 +1015,7 @@
 
 $.ajax({
 url: 
createURL('uploadSslCert'),
+type: "POST",
 data: data,
 success: function(json) {
 var item = 
json.uploadsslcertresponse.sslcert;


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[cloudstack] branch 4.11 updated: ui: Fixes #2558 use POST for uploadSslCert API request (#2661)

[rohit]

This is an automated email from the ASF dual-hosted git repository.

rohit pushed a commit to branch 4.11
in repository https://gitbox.apache.org/repos/asf/cloudstack.git


The following commit(s) were added to refs/heads/4.11 by this push:
 new eb23d91  ui: Fixes #2558 use POST for uploadSslCert API request (#2661)
eb23d91 is described below

commit eb23d91cf465c13e79e7b612258e9614669de7c9
Author: Gabriel Beims Bräscher 
AuthorDate: Wed May 23 01:04:30 2018 -0300

ui: Fixes #2558 use POST for uploadSslCert API request (#2661)
---
 ui/scripts/accounts.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/scripts/accounts.js b/ui/scripts/accounts.js
index a9f7381..192c284 100644
--- a/ui/scripts/accounts.js
+++ b/ui/scripts/accounts.js
@@ -1015,6 +1015,7 @@
 
 $.ajax({
 url: 
createURL('uploadSslCert'),
+type: "POST",
 data: data,
 success: function(json) {
 var item = 
json.uploadsslcertresponse.sslcert;

-- 
To stop receiving notification emails like this one, please contact
ro...@apache.org.

[GitHub] rhtyd commented on issue #2661: Make uploadSslCert a POST request instead of a GET

[GitBox]

rhtyd commented on issue #2661: Make uploadSslCert a POST request instead of a 
GET
URL: https://github.com/apache/cloudstack/pull/2661#issuecomment-391213318
 
 
   Lgtm, UI fix. Will merge. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] Jazz9 commented on issue #2650: Ceph RBD Feature flag update

[GitBox]

Jazz9 commented on issue #2650: Ceph RBD Feature flag update
URL: https://github.com/apache/cloudstack/issues/2650#issuecomment-391182904
 
 
   I think it might be best to leave the default action as it currently is. 
Should we have a global setting for ceph.clone.rbdfeaturecode?
   
   Sent from my phone
   
   On 22 May 2018 9:20 PM, Wido den Hollander  wrote:
   
   @Jazz9 Creating is handled by libvirt, but the 
cloning function wasn't implemented before Ubuntu 16.04 was released. I didn't 
check CentOS 7, but I assume the same.
   
   So we will have to statically define the features we want to use. But as we 
still have people running older Ceph clusters we should be cautious.
   
   Do you want to object-map feature enabled?
   
   -
   You are receiving this because you were mentioned.
   Reply to this email directly, view it on 
GitHub,
 or mute the 
thread.
   
   This e-mail is intended solely for the benefit of the addressee(s) and any 
other named recipient. It is confidential and may contain legally privileged or 
confidential information. If you are not the recipient, any use, distribution, 
disclosure or copying of this e-mail is prohibited. The confidentiality and 
legal privilege attached to this communication is not waived or lost by reason 
of the mistaken transmission or delivery to you. If you have received this 
e-mail in error, please notify us immediately.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] GabrielBrascher commented on issue #2558: API uploadSslCert does not work

[GitBox]

GabrielBrascher commented on issue #2558: API uploadSslCert does not work
URL: https://github.com/apache/cloudstack/issues/2558#issuecomment-391177759
 
 
   We are making a GET request, not a POST. I created PR 
[2661](https://github.com/apache/cloudstack/pull/2661) to fix it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] GabrielBrascher opened a new pull request #2661: Make uploadSslCert a POST request instead of a GET

[GitBox]

GabrielBrascher opened a new pull request #2661: Make uploadSslCert a POST 
request instead of a GET
URL: https://github.com/apache/cloudstack/pull/2661
 
 
   If the Request Header Size grows (for instance a cert chain of 8kb) it will 
fail to upload the SSL Cert.
   
   This problem was raised on the issue 
[2558](https://github.com/apache/cloudstack/issues/2558)
   
   ## Description
   
   
   
   
   
   ## Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing 
functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [x] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## GitHub Issue/PRs
   
   
   
   
   
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   
   
   
   
   
   ## Checklist:
   
   
   - [x] I have read the 
[CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md)
 document.
   - [x] My code follows the code style of this project.
   - [ ] My change requires a change to the documentation.
   - [ ] I have updated the documentation accordingly.
   Testing
   - [ ] I have added tests to cover my changes.
   - [ ] All relevant new and existing integration tests have passed.
   - [ ] A full integration testsuite with all test that can run on my 
environment has passed.
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] PaulAngus commented on issue #2545: dedicatePublicIpRange to domain doesn't work

[GitBox]

PaulAngus commented on issue #2545: dedicatePublicIpRange to domain doesn't work
URL: https://github.com/apache/cloudstack/issues/2545#issuecomment-391125196
 
 
   I've confirmed that this behaviour used to work in 4.9.3 (projects would use 
IP dedicated to the domain that the project was in) and no longer works in 
4.11.0  cc @DaanHoogland 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2660: Macaddresses

[GitBox]

rafaelweingartner commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189961784
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   For me, the goal of unit tests is to do exactly what you described, 
validating/checking that methods are workings as they should. What is the 
difference from validating a method manually to validating a method using unit 
tests?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] khos2ow commented on issue #2649: Catch error in Debian packaging script and fail the build

[GitBox]

khos2ow commented on issue #2649: Catch error in Debian packaging script and 
fail the build
URL: https://github.com/apache/cloudstack/pull/2649#issuecomment-391000806
 
 
   @rhtyd the fact that debian build is failing on BO might actually be a good 
result, that the proposed enhancement in this PR works! Can you check/share the 
log of it?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2660: Macaddresses

[GitBox]

DaanHoogland commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189909270
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   I don't think I am making a strong argument but I do think main methods 
'laying around' are very usefull for all sorts of things; validating ip (6v) 
addresses, validating mac addresses or encrypting/decrypting are but a few 
examples of command line tools that are usefull and one would like to have 
using the same codebase as the core system.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] wido commented on issue #2650: Ceph RBD Feature flag update

[GitBox]

wido commented on issue #2650: Ceph RBD Feature flag update
URL: https://github.com/apache/cloudstack/issues/2650#issuecomment-390987002
 
 
   @Jazz9 Creating is handled by libvirt, but the cloning function wasn't 
implemented before Ubuntu 16.04 was released. I didn't check CentOS 7, but I 
assume the same.
   
   So we will have to statically define the features we want to use. But as we 
still have people running older Ceph clusters we should be cautious.
   
   Do you want to object-map feature enabled?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] Jazz9 commented on issue #2650: Ceph RBD Feature flag update

[GitBox]

Jazz9 commented on issue #2650: Ceph RBD Feature flag update
URL: https://github.com/apache/cloudstack/issues/2650#issuecomment-390973418
 
 
   O no! How are the new volumes created? They seem to get the correct feature 
flags.
   Kind regards,
   Glen Baars
   
   From: Wido den Hollander 
   Sent: Tuesday, 22 May 2018 8:17 PM
   To: apache/cloudstack 
   Cc: Glen Baars ; Mention 

   Subject: Re: [apache/cloudstack] Ceph RBD Feature flag update (#2650)
   
   
   I just found out, libvirt in Ubuntu 16.04 doesn't support this yet, so it 
will take a long time (years) before we can put this upstream as we have to 
support Ubuntu 16.04 until at least 2021.
   
   —
   You are receiving this because you were mentioned.
   Reply to this email directly, view it on 
GitHub,
 or mute the 
thread.
   
   This e-mail is intended solely for the benefit of the addressee(s) and any 
other named recipient. It is confidential and may contain legally privileged or 
confidential information. If you are not the recipient, any use, distribution, 
disclosure or copying of this e-mail is prohibited. The confidentiality and 
legal privilege attached to this communication is not waived or lost by reason 
of the mistaken transmission or delivery to you. If you have received this 
e-mail in error, please notify us immediately.
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sureshanaparti commented on issue #2659: removed unused code in snapshotDao

[GitBox]

sureshanaparti commented on issue #2659: removed unused code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#issuecomment-390970602
 
 
   @DaanHoogland Not related to dao. I observed that the Event 
"BackedupToSecondary" in _Snapshot_ class is never used. Please take a look and 
if not required, clean that. Thanks.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] wido commented on issue #2650: Ceph RBD Feature flag update

[GitBox]

wido commented on issue #2650: Ceph RBD Feature flag update
URL: https://github.com/apache/cloudstack/issues/2650#issuecomment-390968920
 
 
   I just found out, libvirt in Ubuntu 16.04 doesn't support this yet, so it 
will take a long time (years) before we can put this upstream as we have to 
support Ubuntu 16.04 until at least 2021.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] wido commented on issue #2657: KVM hosts are being reset by a script

[GitBox]

wido commented on issue #2657: KVM hosts are being reset by a script
URL: https://github.com/apache/cloudstack/issues/2657#issuecomment-390968718
 
 
   The main problem is that it is hard to determine if the host isn't 
responding or that something else is going on.
   
   You do not want the VM to run twice as that might (will!) cause data 
corruption. That's why a hard reboot is being issued if the storage is having 
trouble.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[cloudstack] 01/01: Merge branch '4.11'

[rohit]

This is an automated email from the ASF dual-hosted git repository.

rohit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git

commit ada1e730c82da2ffd9c565a43c1617aff920ad89
Merge: 9c1eabf 8b09620
Author: Rohit Yadav 
AuthorDate: Tue May 22 17:03:24 2018 +0530

Merge branch '4.11'

 .../admin/volume/ListVolumesCmdByAdmin.java|  39 +--
 .../api/command/user/volume/ListVolumesCmd.java|  33 +-
 .../java/com/cloud/api/query/QueryManagerImpl.java | 353 -
 3 files changed, 153 insertions(+), 272 deletions(-)


-- 
To stop receiving notification emails like this one, please contact
ro...@apache.org.

[cloudstack] branch 4.11 updated: CLOUDSTACK-10276: listVolumes not working when storage UUID is not a UUID (#2639)

[rohit]

This is an automated email from the ASF dual-hosted git repository.

rohit pushed a commit to branch 4.11
in repository https://gitbox.apache.org/repos/asf/cloudstack.git


The following commit(s) were added to refs/heads/4.11 by this push:
 new 8b09620  CLOUDSTACK-10276: listVolumes not working when storage UUID 
is not a UUID (#2639)
8b09620 is described below

commit 8b09620d777805bdbb59255ef722e0a6ddbb85b8
Author: Rafael Weingärtner 
AuthorDate: Tue May 22 08:32:40 2018 -0300

CLOUDSTACK-10276: listVolumes not working when storage UUID is not a UUID 
(#2639)

When configuring a pre-setup primary storage we can enter the name-label of 
the storage that is going to be used by ACS and is already set up in the host. 
The problem is that we can use any String of characters there, and this String 
does not need to be a UUID. When listing volumes from a primary storage that 
has such conditions, the list will return all of the volumes in the cloud 
because the “API framework” will ignore that value as it is not a UUID type.
---
 .../admin/volume/ListVolumesCmdByAdmin.java|  39 +--
 .../api/command/user/volume/ListVolumesCmd.java|  33 +-
 .../src/com/cloud/api/query/QueryManagerImpl.java  | 353 -
 3 files changed, 153 insertions(+), 272 deletions(-)

diff --git 
a/api/src/org/apache/cloudstack/api/command/admin/volume/ListVolumesCmdByAdmin.java
 
b/api/src/org/apache/cloudstack/api/command/admin/volume/ListVolumesCmdByAdmin.java
index 5fe5bfe..add2271 100644
--- 
a/api/src/org/apache/cloudstack/api/command/admin/volume/ListVolumesCmdByAdmin.java
+++ 
b/api/src/org/apache/cloudstack/api/command/admin/volume/ListVolumesCmdByAdmin.java
@@ -16,50 +16,15 @@
 // under the License.
 package org.apache.cloudstack.api.command.admin.volume;
 
-import org.apache.log4j.Logger;
-
-import org.apache.cloudstack.acl.RoleType;
 import org.apache.cloudstack.api.APICommand;
-import org.apache.cloudstack.api.ApiConstants;
-import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.ResponseObject.ResponseView;
 import org.apache.cloudstack.api.command.user.volume.ListVolumesCmd;
-import org.apache.cloudstack.api.response.PodResponse;
-import org.apache.cloudstack.api.response.StoragePoolResponse;
 import org.apache.cloudstack.api.response.VolumeResponse;
 
 import com.cloud.storage.Volume;
 
-
-@APICommand(name = "listVolumes", description = "Lists all volumes.", 
responseObject = VolumeResponse.class, responseView = ResponseView.Full, 
entityType = {Volume.class},
-requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+@APICommand(name = "listVolumes", description = "Lists all volumes.", 
responseObject = VolumeResponse.class, responseView = ResponseView.Full, 
entityType = {
+Volume.class}, requestHasSensitiveInfo = false, 
responseHasSensitiveInfo = false)
 public class ListVolumesCmdByAdmin extends ListVolumesCmd {
-public static final Logger s_logger = 
Logger.getLogger(ListVolumesCmdByAdmin.class.getName());
-
-@Parameter(name=ApiConstants.POD_ID, type=CommandType.UUID, 
entityType=PodResponse.class,
-description="the pod id the disk volume belongs to")
-private Long podId;
-
-
-@Parameter(name=ApiConstants.STORAGE_ID, type=CommandType.UUID, 
entityType=StoragePoolResponse.class,
-description="the ID of the storage pool, available to ROOT admin 
only", since="4.3", authorized = { RoleType.Admin })
-private Long storageId;
-
-
-/
-/// Accessors ///
-/
-
-
-@Override
-public Long getPodId() {
-return podId;
-}
-
-
-@Override
-public Long getStorageId() {
-return storageId;
-}
 
 }
diff --git 
a/api/src/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java 
b/api/src/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java
index 554e029..c858f49 100644
--- a/api/src/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java
+++ b/api/src/org/apache/cloudstack/api/command/user/volume/ListVolumesCmd.java
@@ -16,9 +16,8 @@
 // under the License.
 package org.apache.cloudstack.api.command.user.volume;
 
-import org.apache.log4j.Logger;
-
 import java.util.List;
+
 import org.apache.cloudstack.acl.RoleType;
 import org.apache.cloudstack.api.APICommand;
 import org.apache.cloudstack.api.ApiCommandJobType;
@@ -35,11 +34,12 @@ import 
org.apache.cloudstack.api.response.StoragePoolResponse;
 import org.apache.cloudstack.api.response.UserVmResponse;
 import org.apache.cloudstack.api.response.VolumeResponse;
 import org.apache.cloudstack.api.response.ZoneResponse;
+import org.apache.log4j.Logger;
 
 import com.cloud.storage.Volume;
 
-@APICommand(name = "listVolumes", description = "Lists all volumes.", 
responseObject = VolumeResponse.class, responseView 

[cloudstack] branch master updated (9c1eabf -> ada1e73)

[rohit]

This is an automated email from the ASF dual-hosted git repository.

rohit pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git.


from 9c1eabf  Merge branch '4.11'
 add 8b09620  CLOUDSTACK-10276: listVolumes not working when storage UUID 
is not a UUID (#2639)
 new ada1e73  Merge branch '4.11'

The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 .../admin/volume/ListVolumesCmdByAdmin.java|  39 +--
 .../api/command/user/volume/ListVolumesCmd.java|  33 +-
 .../java/com/cloud/api/query/QueryManagerImpl.java | 353 -
 3 files changed, 153 insertions(+), 272 deletions(-)

-- 
To stop receiving notification emails like this one, please contact
ro...@apache.org.

[GitHub] blueorangutan commented on issue #2660: Macaddresses

[GitBox]

blueorangutan commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390956360
 
 
   Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2075


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

DaanHoogland commented on a change in pull request #2659: removed unused code 
in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189864373
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -214,26 +183,6 @@ protected void init() {
 InstanceIdSearch.done();
 }
 
-@Override
-public Long getSecHostId(long volumeId) {
-
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = GET_SECHOST_ID;
 
 Review comment:
   done


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

DaanHoogland commented on a change in pull request #2659: removed unused code 
in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189864397
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -253,41 +202,6 @@ public long getLastSnapshot(long volumeId, DataStoreRole 
role) {
 return 0;
 }
 
-@Override
-public long updateSnapshotVersion(long volumeId, String from, String to) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SNAPSHOT_VERSION;
 
 Review comment:
   done


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

DaanHoogland commented on a change in pull request #2659: removed unused code 
in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189864415
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -253,41 +202,6 @@ public long getLastSnapshot(long volumeId, DataStoreRole 
role) {
 return 0;
 }
 
-@Override
-public long updateSnapshotVersion(long volumeId, String from, String to) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SNAPSHOT_VERSION;
-try {
-pstmt = txn.prepareAutoCloseStatement(sql);
-pstmt.setString(1, to);
-pstmt.setLong(2, volumeId);
-pstmt.setString(3, from);
-pstmt.executeUpdate();
-return 1;
-} catch (Exception ex) {
-s_logger.error("error getting last snapshot", ex);
-}
-return 0;
-}
-
-@Override
-public long updateSnapshotSecHost(long dcId, long secHostId) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SECHOST_ID;
 
 Review comment:
   done


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2639: [CLOUDSTACK-10276] listVolumes not working when storage UUID is not a UUID

[GitBox]

DaanHoogland commented on issue #2639: [CLOUDSTACK-10276] listVolumes not 
working when storage UUID is not a UUID
URL: https://github.com/apache/cloudstack/pull/2639#issuecomment-390955253
 
 
   @rafaelweingartner I am fine with merging this so far as we don't allow for 
manually entering a non-uuid storage id. In my understanding it doesn't. Is 
that correct?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] blueorangutan commented on issue #2659: removed unused code in snapshotDao

[GitBox]

blueorangutan commented on issue #2659: removed unused code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#issuecomment-390950430
 
 
   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you 
posted as I make progress.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2659: removed unused code in snapshotDao

[GitBox]

DaanHoogland commented on issue #2659: removed unused code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#issuecomment-390950370
 
 
   @blueorangutan package


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2653: Generate MAC address if the MAC in command addNicToVirtualMachine is invalid

[GitBox]

rafaelweingartner commented on a change in pull request #2653: Generate MAC 
address if the MAC in command addNicToVirtualMachine is invalid
URL: https://github.com/apache/cloudstack/pull/2653#discussion_r189858968
 
 

 ##
 File path: server/src/com/cloud/vm/UserVmManagerImpl.java
 ##
 @@ -1228,6 +1230,19 @@ public UserVm addNicToVirtualMachine(AddNicToVMCmd cmd) 
throws InvalidParameterV
 return _vmDao.findById(vmInstance.getId());
 }
 
+/**
+ * If the given MAC address is invalid it replaces the given MAC with the 
next available MAC address
+ */
+protected String validateOrReplaceMacAddress(String macAddress, long 
networkId) {
+if (!NetUtils.isValidMac(macAddress)) {
 
 Review comment:
   Well.. yes and no. It depends on how the API is handling this case. I am not 
sure if all (`blank`, `empty`, `null`) will be represented as the same. I mean, 
I do not know if whenever we enter a blank or empty case, if these values will 
be treated as `null`  by ACS.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2660: Macaddresses

[GitBox]

rafaelweingartner commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189857791
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   I still do not get it... I think we should not have "main" methods laying 
around. If it is a test worth having, it should be a unit test case. These 
"main" methods that we have, they all seem to be left overs that programmers 
"forget" when pushing code.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

DaanHoogland commented on a change in pull request #2659: removed unused code 
in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189857894
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -253,41 +202,6 @@ public long getLastSnapshot(long volumeId, DataStoreRole 
role) {
 return 0;
 }
 
-@Override
-public long updateSnapshotVersion(long volumeId, String from, String to) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SNAPSHOT_VERSION;
-try {
-pstmt = txn.prepareAutoCloseStatement(sql);
-pstmt.setString(1, to);
-pstmt.setLong(2, volumeId);
-pstmt.setString(3, from);
-pstmt.executeUpdate();
-return 1;
-} catch (Exception ex) {
-s_logger.error("error getting last snapshot", ex);
-}
-return 0;
-}
-
-@Override
-public long updateSnapshotSecHost(long dcId, long secHostId) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SECHOST_ID;
 
 Review comment:
   thanks, will do


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2653: Generate MAC address if the MAC in command addNicToVirtualMachine is invalid

[GitBox]

DaanHoogland commented on a change in pull request #2653: Generate MAC address 
if the MAC in command addNicToVirtualMachine is invalid
URL: https://github.com/apache/cloudstack/pull/2653#discussion_r189857586
 
 

 ##
 File path: server/src/com/cloud/vm/UserVmManagerImpl.java
 ##
 @@ -1228,6 +1230,19 @@ public UserVm addNicToVirtualMachine(AddNicToVMCmd cmd) 
throws InvalidParameterV
 return _vmDao.findById(vmInstance.getId());
 }
 
+/**
+ * If the given MAC address is invalid it replaces the given MAC with the 
next available MAC address
+ */
+protected String validateOrReplaceMacAddress(String macAddress, long 
networkId) {
+if (!NetUtils.isValidMac(macAddress)) {
 
 Review comment:
   isn't empty or blank the same as null from a user perspective? It is up to 
the API-client how it passes no value. (we agree on invalid completely)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] blueorangutan commented on issue #2660: Macaddresses

[GitBox]

blueorangutan commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390947798
 
 
   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you 
posted as I make progress.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2660: Macaddresses

[GitBox]

DaanHoogland commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390947600
 
 
   @blueorangutan package


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2660: Macaddresses

[GitBox]

DaanHoogland commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189856988
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   :) a test mac addresses utility ???


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2660: Macaddresses

[GitBox]

DaanHoogland commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390932472
 
 
   @blueorangutan package


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

rafaelweingartner commented on a change in pull request #2659: removed unused 
code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189846178
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -214,26 +183,6 @@ protected void init() {
 InstanceIdSearch.done();
 }
 
-@Override
-public Long getSecHostId(long volumeId) {
-
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = GET_SECHOST_ID;
 
 Review comment:
   The `GET_SECHOST_ID` variable is also only used here. So, what about if you 
delete it as well?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

rafaelweingartner commented on a change in pull request #2659: removed unused 
code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189846308
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -253,41 +202,6 @@ public long getLastSnapshot(long volumeId, DataStoreRole 
role) {
 return 0;
 }
 
-@Override
-public long updateSnapshotVersion(long volumeId, String from, String to) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SNAPSHOT_VERSION;
 
 Review comment:
   The same happens here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2659: removed unused code in snapshotDao

[GitBox]

rafaelweingartner commented on a change in pull request #2659: removed unused 
code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#discussion_r189846471
 
 

 ##
 File path: 
engine/schema/src/main/java/com/cloud/storage/dao/SnapshotDaoImpl.java
 ##
 @@ -253,41 +202,6 @@ public long getLastSnapshot(long volumeId, DataStoreRole 
role) {
 return 0;
 }
 
-@Override
-public long updateSnapshotVersion(long volumeId, String from, String to) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SNAPSHOT_VERSION;
-try {
-pstmt = txn.prepareAutoCloseStatement(sql);
-pstmt.setString(1, to);
-pstmt.setLong(2, volumeId);
-pstmt.setString(3, from);
-pstmt.executeUpdate();
-return 1;
-} catch (Exception ex) {
-s_logger.error("error getting last snapshot", ex);
-}
-return 0;
-}
-
-@Override
-public long updateSnapshotSecHost(long dcId, long secHostId) {
-TransactionLegacy txn = TransactionLegacy.currentTxn();
-PreparedStatement pstmt = null;
-String sql = UPDATE_SECHOST_ID;
 
 Review comment:
   and here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2660: Macaddresses

[GitBox]

rafaelweingartner commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189844708
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   hmm.. So, what does justify a "main" method in the test class?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2660: Macaddresses

[GitBox]

DaanHoogland commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189841833
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   it was just a move, and the logic didn't seem to justify a unit test


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2660: Macaddresses

[GitBox]

DaanHoogland commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390932472
 
 
   @blueorangutan package


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on issue #2639: [CLOUDSTACK-10276] listVolumes not working when storage UUID is not a UUID

[GitBox]

rafaelweingartner commented on issue #2639: [CLOUDSTACK-10276] listVolumes not 
working when storage UUID is not a UUID
URL: https://github.com/apache/cloudstack/pull/2639#issuecomment-390929227
 
 
   I really do not see any other smaller/simpler solution for this issue. 
Moreover, it does not affect/break anything in the client side. The client is 
already seeing an ID, which is a UUID.
   
   The changes are only internal to ACS. I changed the type of `storageId` from 
`long` to `String` to avoid using that method that converts the given UUID to 
an internal database ID. That method is validating the input, and ignoring 
anything that is not an UUID. Then, I am using the UUID as the filter at 
`QueryManagerImpl.java`.
   
   The changed lines are only 2. The other lines are a result of a code cleanup 
that I did, and the code formatting I applied.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2653: Generate MAC address if the MAC in command addNicToVirtualMachine is invalid

[GitBox]

rafaelweingartner commented on a change in pull request #2653: Generate MAC 
address if the MAC in command addNicToVirtualMachine is invalid
URL: https://github.com/apache/cloudstack/pull/2653#discussion_r189836738
 
 

 ##
 File path: server/src/com/cloud/vm/UserVmManagerImpl.java
 ##
 @@ -1228,6 +1230,19 @@ public UserVm addNicToVirtualMachine(AddNicToVMCmd cmd) 
throws InvalidParameterV
 return _vmDao.findById(vmInstance.getId());
 }
 
+/**
+ * If the given MAC address is invalid it replaces the given MAC with the 
next available MAC address
+ */
+protected String validateOrReplaceMacAddress(String macAddress, long 
networkId) {
+if (!NetUtils.isValidMac(macAddress)) {
 
 Review comment:
   I think we should only generate a MAC address if the value entered by the 
user is null. All other combinations (invalid, empty, blank and so on) should 
return an invalid parameter value exception.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rafaelweingartner commented on a change in pull request #2660: Macaddresses

[GitBox]

rafaelweingartner commented on a change in pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#discussion_r189834173
 
 

 ##
 File path: utils/src/test/java/com/cloud/utils/net/MacAddressTest.java
 ##
 @@ -51,10 +52,11 @@ public final void testMacAddressToLong() throws Exception {
 // TODOpublic final void testToString() throws Exception {
 // TODOpublic final void testGetMacAddress() throws Exception {
 // TODOpublic final void testParse() throws Exception {
-// TODOpublic final void testMain() throws Exception {
-// TODOpublic final void testParseLong() throws Exception {
-// TODOpublic final void testParseInt() throws Exception {
-// TODOpublic final void testParseShort() throws Exception {
-// TODOpublic final void testParseByte() throws Exception {
 
+public static void main(String[] args) {
 
 Review comment:
   A "main" method in a test file? Why not create unit test cases instead?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] blueorangutan commented on issue #2660: Macaddresses

[GitBox]

blueorangutan commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390918511
 
 
   Packaging result: ✖centos6 ✖centos7 ✖debian. JID-2074


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] blueorangutan commented on issue #2659: removed unused code in snapshotDao

[GitBox]

blueorangutan commented on issue #2659: removed unused code in snapshotDao
URL: https://github.com/apache/cloudstack/pull/2659#issuecomment-390918506
 
 
   Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2073


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] blueorangutan commented on issue #2660: Macaddresses

[GitBox]

blueorangutan commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390910591
 
 
   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you 
posted as I make progress.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2660: Macaddresses

[GitBox]

DaanHoogland commented on issue #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660#issuecomment-390910499
 
 
   @blueorangutan package


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland opened a new pull request #2660: Macaddresses

[GitBox]

DaanHoogland opened a new pull request #2660: Macaddresses
URL: https://github.com/apache/cloudstack/pull/2660
 
 
   ## Description
   
   
   
   
   
   ## Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing 
functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [x] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## GitHub Issue/PRs
   
   
   
   
   
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   
   
   
   
   
   ## Checklist:
   
   
   - [ ] I have read the 
[CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md)
 document.
   - [ ] My code follows the code style of this project.
   - [ ] My change requires a change to the documentation.
   - [ ] I have updated the documentation accordingly.
   Testing
   - [ ] I have added tests to cover my changes.
   - [ ] All relevant new and existing integration tests have passed.
   - [ ] A full integration testsuite with all test that can run on my 
environment has passed.
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] blueorangutan commented on issue #2659: removed unused code

[GitBox]

blueorangutan commented on issue #2659: removed unused code
URL: https://github.com/apache/cloudstack/pull/2659#issuecomment-390910283
 
 
   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you 
posted as I make progress.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2659: removed unused code

[GitBox]

DaanHoogland commented on issue #2659: removed unused code
URL: https://github.com/apache/cloudstack/pull/2659#issuecomment-390910097
 
 
   @blueorangutan package


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland opened a new pull request #2659: removed unused code

[GitBox]

DaanHoogland opened a new pull request #2659: removed unused code
URL: https://github.com/apache/cloudstack/pull/2659
 
 
   ## Description
   
   
   
   
   
   ## Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing 
functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [ ] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [x] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ## GitHub Issue/PRs
   
   
   
   
   
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   
   
   
   
   
   ## Checklist:
   
   
   - [ ] I have read the 
[CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md)
 document.
   - [ ] My code follows the code style of this project.
   - [ ] My change requires a change to the documentation.
   - [ ] I have updated the documentation accordingly.
   Testing
   - [ ] I have added tests to cover my changes.
   - [ ] All relevant new and existing integration tests have passed.
   - [ ] A full integration testsuite with all test that can run on my 
environment has passed.
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on a change in pull request #2653: Generate MAC address if the MAC in command addNicToVirtualMachine is invalid

[GitBox]

DaanHoogland commented on a change in pull request #2653: Generate MAC address 
if the MAC in command addNicToVirtualMachine is invalid
URL: https://github.com/apache/cloudstack/pull/2653#discussion_r189795031
 
 

 ##
 File path: server/src/com/cloud/vm/UserVmManagerImpl.java
 ##
 @@ -1228,6 +1230,19 @@ public UserVm addNicToVirtualMachine(AddNicToVMCmd cmd) 
throws InvalidParameterV
 return _vmDao.findById(vmInstance.getId());
 }
 
+/**
+ * If the given MAC address is invalid it replaces the given MAC with the 
next available MAC address
+ */
+protected String validateOrReplaceMacAddress(String macAddress, long 
networkId) {
+if (!NetUtils.isValidMac(macAddress)) {
 
 Review comment:
   I am having second thoughts, when a user entered a MAC address but made a 
typo, we are now creating a new address for them, uncalled for. shouldn't we 
have refused and thrown some horrible thing instead?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] wido commented on issue #2472: CLOUDSTACK-10310 Fix KVM reboot on storage issue

[GitBox]

wido commented on issue #2472: CLOUDSTACK-10310 Fix KVM reboot on storage issue
URL: https://github.com/apache/cloudstack/pull/2472#issuecomment-390883172
 
 
   @rhtyd Both cases are not ideal. Because if you stop the Agent due to a 
single Primary Storage failure you leave VMs running there which still might be 
doing I/O and to start them a second time somewhere else.
   
   The hard reboot isn't a very nice either, so you have to deal with two bad 
things.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rhtyd commented on issue #2639: [CLOUDSTACK-10276] listVolumes not working when storage UUID is not a UUID

[GitBox]

rhtyd commented on issue #2639: [CLOUDSTACK-10276] listVolumes not working when 
storage UUID is not a UUID
URL: https://github.com/apache/cloudstack/pull/2639#issuecomment-390881251
 
 
   I'm unable to find time to do the investigation on this. @DaanHoogland can 
you advise, and if it's all okay please go ahead with merging this.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] rhtyd commented on issue #2558: API uploadSslCert does not work

[GitBox]

rhtyd commented on issue #2558: API uploadSslCert does not work
URL: https://github.com/apache/cloudstack/issues/2558#issuecomment-390879871
 
 
   @GabrielBrascher can you at least help test this against a normal 
certificate of size less than 8k? Usually certificates are 2k-4k in size. If 
there are no blockers around using custom certificates, then yes we can discuss 
pushing it for 4.11.2.0 milestone.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2558: API uploadSslCert does not work

[GitBox]

DaanHoogland commented on issue #2558: API uploadSslCert does not work
URL: https://github.com/apache/cloudstack/issues/2558#issuecomment-390877241
 
 
   :) -(a weekend)
   The question is not how much time you have but how much we need this in, 
@GabrielBrascher 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] DaanHoogland commented on issue #2472: CLOUDSTACK-10310 Fix KVM reboot on storage issue

[GitBox]

DaanHoogland commented on issue #2472: CLOUDSTACK-10310 Fix KVM reboot on 
storage issue
URL: https://github.com/apache/cloudstack/pull/2472#issuecomment-390876820
 
 
   echo b > /proc/sysrq-trigger is not acceptable to more people, I don't see 
how killing (and maybe restarting) the agent can be worse. It might not solve 
issues related to nfs for instance.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services