[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user asfgit closed the pull request at: https://github.com/apache/cloudstack/pull/41 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user karuturi commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/41#discussion_r20492616 --- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java --- @@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop type = c.getType(); } +String errMsg = null; +try { +if (type.equals(Integer.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid integer value for parameter + name; +Integer.parseInt(value); +} else if (type.equals(Float.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid float value for parameter + name; +Float.parseFloat(value); --- End diff -- I understand that it will fail during parsing. But, we can string replace , with --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user karuturi commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/41#discussion_r20492860 --- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java --- @@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop type = c.getType(); } +String errMsg = null; +try { +if (type.equals(Integer.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid integer value for parameter + name; +Integer.parseInt(value); +} else if (type.equals(Float.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid float value for parameter + name; +Float.parseFloat(value); +} +} catch (Exception e) { --- End diff -- catching top level exception and swallowing the stacktrace makes it really difficult during debugging. For end user, we anyway wont show the exception but only the message. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
GitHub user anshul1886 opened a pull request: https://github.com/apache/cloudstack/pull/41 CLOUDSTACK-7930, CLOUDSTACK-7931: Do not allow to set invalid values for global settings which are of type integer and float Do not allow to set invalid values for global settings which are of type integer and float You can merge this pull request into a Git repository by running: $ git pull https://github.com/anshul1886/cloudstack-1 CLOUDSTACK-7930 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/41.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #41 commit b7de1ecbe470365c30bc6afcea5f5c560bc9c8c2 Author: Anshul Gangwar anshul.gang...@citrix.com Date: 2014-11-17T09:57:22Z CLOUDSTACK-7930, CLOUDSTACK-7931: Do not allow to set invalid values for global settings which are of type integer and float --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user karuturi commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/41#discussion_r20488370 --- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java --- @@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop type = c.getType(); } +String errMsg = null; +try { +if (type.equals(Integer.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid integer value for parameter + name; +Integer.parseInt(value); +} else if (type.equals(Float.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid float value for parameter + name; +Float.parseFloat(value); --- End diff -- should we allow values like 3,400.5 ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user karuturi commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/41#discussion_r20488404 --- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java --- @@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop type = c.getType(); } +String errMsg = null; +try { +if (type.equals(Integer.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid integer value for parameter + name; +Integer.parseInt(value); +} else if (type.equals(Float.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid float value for parameter + name; +Float.parseFloat(value); +} +} catch (Exception e) { --- End diff -- Can you catch specific expected exceptions NPE and NFE? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user karuturi commented on the pull request: https://github.com/apache/cloudstack/pull/41#issuecomment-63430386 Can you add unittests for this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user anshul1886 commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/41#discussion_r20489652 --- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java --- @@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop type = c.getType(); } +String errMsg = null; +try { +if (type.equals(Integer.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid integer value for parameter + name; +Integer.parseInt(value); +} else if (type.equals(Float.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid float value for parameter + name; +Float.parseFloat(value); --- End diff -- No. Because they will fail later in parsing. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-7930, CLOUDSTACK-7931: Do not ...
Github user anshul1886 commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/41#discussion_r20489860 --- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java --- @@ -725,6 +725,21 @@ private String validateConfigurationValue(String name, String value, String scop type = c.getType(); } +String errMsg = null; +try { +if (type.equals(Integer.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid integer value for parameter + name; +Integer.parseInt(value); +} else if (type.equals(Float.class)) { +errMsg = There was error in trying to parse value: + value + . Please enter a valid float value for parameter + name; +Float.parseFloat(value); +} +} catch (Exception e) { --- End diff -- What value will be added by catching specific exceptions? In the end user only wants to know whether the value is valid or not. he/she is not concerned with what type of exception he is getting. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---