[
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)
