CLOUDSTACK-9132: API createVolume takes empty string for name parameter #1319
Conversation
|
Reference PR #1273 (Against master) with 3 LGTMs. This PR (against 4.7) contains the same code change. |
Added conditions to check if the name is empty or blank. If it is empty or blank, then it generates a random name. Made the name field as optional in UI as well as in API. Added required unit tests.
|
Hi @remibergsma, I created all my pending PRs against 4.7. Can you please review once. Thanks. |
|
@remibergsma : Any updates on this. Thanks |
|
Just curious. If this is the expected behavior (mentioned above): It shouldn't create a volume with an empty name. Error should be returned. Then it doesn't sound like this PR solves that issue as it generates a random name. Did we decide at some point that a random name was the preferred solution? Thanks! |
|
@mike-tutkowski : Yes, we decided to make random name as a preferred solution. You can refer the detail conversation on this PR #1273 . Thanks for reviewing. |
There was a problem hiding this comment.
The code LGTM (I did not test it...just reviewed the code). My only comment is on this line. Curious why we don't provide the import at the top as is standard in CloudStack. Not a big deal, but it makes it easier to see at a glance what all packages are involved. Perhaps there is another StringUtils being imported, so this had to be done here?
There was a problem hiding this comment.
@mike-tutkowski : Exactly there is one more StringUtils. If you put this it will conflict. I tried in that way but it gives error.
There was a problem hiding this comment.
Cool...then it all looks good to me.
Unfortunately, at the time being, I have my dev system in a state that would make it difficult and time consuming for me to test this.
If you do require a tester, please let me know. It might take a few days, though, before I finish up my current project and can put this code on my system.
Thanks
There was a problem hiding this comment.
Yes, you can test the change. But I have tested this on my environment. Its working fine. Thanks.
|
@remibergsma @DaanHoogland : It has got 4 LGTMs. Is it going to be merged? Should it be reviewed by some more people? Thanks. |
|
@nitin-maharana I see two LGTM in the other PR based on code review. One without explanation and none based on testing. I can queue this but you don't want to wait on my backlog in this perspect :( |
|
@nitin-maharana We can merge this once the integration tests have run. Will put it on my list. |
|
Sure @remi |
|
At leaseweb we noticed the same behavior for networks. I propose we use the uuid as the name there as well. Will create a PR. |
There was a problem hiding this comment.
I didn't notice this until I started working on a similar issue for createNetwork. Shouldn't this be in a dictionary somewhere? @milamberspace You are the expert. Can you give an opinion?
There was a problem hiding this comment.
Yes that would be better to put this sentences into dictionary and messages.properties (key should start with "message.tooltip.xxxx")
(even if they are a lot of 'desc' strings in javascript files which need to be localized. Probably a work for me ;-))
There was a problem hiding this comment.
Ah, so in the present state you do not consider this worth a 👎 , I read from your comment.
There was a problem hiding this comment.
Yes will not a -1. Better if the string is localize in the current PR, but not a -1 if not.
|
LGTM, the one error in the files below is a known error in the test environment and unrelated. This has been reviewed extensively. |
CLOUDSTACK-9132: API createVolume takes empty string for name parameterSteps 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. Solution: ======= Added a condition to check in case of empty string. If the name is an empty string, it generates a random name for the volume. Made the name field optional in UI as well as in API. * pr/1319: CLOUDSTACK-9132: API createVolume takes empty string for name parameter Signed-off-by: Remi Bergsma <github@remi.nl>
6 participants
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.
Solution:
Added a condition to check in case of empty string. If the name is an empty string, it generates a random name for the volume. Made the name field optional in UI as well as in API.