cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLOUDSTACK-9132) API createVolume takes empty string for name parameter
Date Wed, 23 Dec 2015 20:51:46 GMT

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-9132?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15070182#comment-15070182
] 

ASF GitHub Bot commented on CLOUDSTACK-9132:
--------------------------------------------

Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1273#issuecomment-166987980
  
    @nitin-maharana, thanks for the update, I will just call your attention to a few points:
    
    First of all, why did you open a new PR? I know you closed the other one, but IMHO it
makes my job harder. The other PR had all of our conversions; you should have kept working
there. It would be easier to track everything that was discussed if everything was at the
same PR. Now please, keep with this one, no need to come back to the one you already closed
or to open a new one after my suggestions.
    
    I noticed that you extract the code to a method as I suggested and created some test cases.
You also add the “required = true” as Daan suggested; that is good.
    
    Now about the method “getVolumeNameFromCommand” you created. Please note that // is
not the proper way to create a Java doc, please replace the // by the proper mark of Java
docs
    
    /** 
    * this is a java doc
    **/
    
    /*
    *This is a block comment
    */
    
    // this is a line comment
    
    About the Java doc I would also encourage you to detail a little bit more the method such
as:
    /** 
    * This method retrieves the volume name from the CreateVolumeCmd object.
    * If the method CreateVolumeCmd#getVolumeName() returns null, empty or blank, It will
be generated a random name using getRandomVolumeName().
    **/
    
    I do not know the proper notation of java doc to make reference to java classes and methods,
so you will need to google it.
    
    Now about the tests, please remove those comments over method names. I believe they are
not needed as the test method name is self-explained.
    
    About the “docs.js”, please improve the sentence you wrote. 'Enter a unique volume
name; if a name is not provided, a random name will be created'
    
    Now, I draw your attention again to the idea of sticking to the same PR were we started
reviewing and chatting. If another reviewer comes to help with this new PR, she/he would not
know about the blank case I told you in the other PR. The reviewer might even give a LGTM
without even noticing that this PR is incomplete.
    Having said that, could you add a test case to check the case in which users enter blank
strings in the volume name? And of course, fix that in the code; otherwise, the test methods
would not work.
     



> API createVolume takes empty string for name parameter
> ------------------------------------------------------
>
>                 Key: CLOUDSTACK-9132
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9132
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the default.) 
>          Components: API
>            Reporter: Nitin Kumar Maharana
>
> Steps to Reproduce:
> ================
> Create a volume using createVolume API where parameter name is empty.
> It creates a volume with empty name.
> But the name parameter is mandatory.(Issue)
> Expected Behaviour:
> ================
> It shouldn't create a volume with an empty name. Error should be returned.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message