Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1319
---
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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-172807056
LGTM, the one error in the files below is a known error in the test
environment and unrelated. This has been reviewed extensively.
[1319.network.resul
Github user milamberspace commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r50086554
--- Diff: ui/scripts/docs.js ---
@@ -1008,7 +1008,7 @@ cloudStack.docs = {
},
// Add volume
helpVolumeName: {
-
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r50081550
--- Diff: ui/scripts/docs.js ---
@@ -1008,7 +1008,7 @@ cloudStack.docs = {
},
// Add volume
helpVolumeName: {
-
Github user milamberspace commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r50079832
--- Diff: ui/scripts/docs.js ---
@@ -1008,7 +1008,7 @@ cloudStack.docs = {
},
// Add volume
helpVolumeName: {
-
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r49997495
--- Diff: ui/scripts/docs.js ---
@@ -1008,7 +1008,7 @@ cloudStack.docs = {
},
// Add volume
helpVolumeName: {
-
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-172522583
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.
---
If your project is set up fo
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-172376856
Sure @remi
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-172376749
@nitin-maharana We can merge this once the integration tests have run. Will
put it on my list.
---
If your project is set up for it, you can reply to this emai
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-171278817
@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
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-171184133
@remibergsma @DaanHoogland : It has got 4 LGTMs. Is it going to be merged?
Should it be reviewed by some more people? Thanks.
---
If your project is set up
Github user nitin-maharana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r49551329
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -476,6 +476,25 @@ public VolumeVO doInTransaction(TransactionStatus
status
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r49498772
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -476,6 +476,25 @@ public VolumeVO doInTransaction(TransactionStatus
status
Github user nitin-maharana commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r49498039
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -476,6 +476,25 @@ public VolumeVO doInTransaction(TransactionStatus
status
Github user mike-tutkowski commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1319#discussion_r49493043
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -476,6 +476,25 @@ public VolumeVO doInTransaction(TransactionStatus
status
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-170825451
@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.
Github user mike-tutkowski commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-170804399
Just curious. If this is the expected behavior (mentioned above):
It shouldn't create a volume with an empty name. Error should be returned.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-170801193
@remibergsma : Any updates on this. Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-169935900
Hi @remibergsma, I created all my pending PRs against 4.7. Can you please
review once. Thanks.
---
If your project is set up for it, you can reply to this e
Github user nitin-maharana closed the pull request at:
https://github.com/apache/cloudstack/pull/1273
---
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 fe
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-169927533
Closing this PR as made a new PR #1319 (Against 4.7 which will be merged in
master later).
---
If your project is set up for it, you can reply to this email
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1319#issuecomment-169927365
Reference PR #1273 (Against master) with 3 LGTMs. This PR (against 4.7)
contains the same code change.
---
If your project is set up for it, you can reply t
GitHub user nitin-maharana opened a pull request:
https://github.com/apache/cloudstack/pull/1319
CLOUDSTACK-9132: API createVolume takes empty string for name parameter
Steps to Reproduce:
Create a volume using createVolume API where parameter name is em
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-168527922
@remibergsma There are internal political issues that people may face with
certain upgrade, be they with or without reason.
---
If your project is set up for
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-168489874
@nitin-maharana See thread on dev-list: the errors are due to quota plugin
failing since Jan 1st 2016. Once this is resolved, you can rebase.
@DaanHoogl
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-168489232
@remibergsma : My new PRs are failing in 4.7. With the same change it was
successful in master. I think there is some issue with the branch.
---
If your pr
Remi, If a fix can be made agains 4.6, let's do that. Merging forward is no
biggy and we'll be helping more people this way.
On Sun, Jan 3, 2016 at 10:39 AM, remibergsma wrote:
> Github user remibergsma commented on the pull request:
>
> https://github.com/apache/cloudstack/pull/1273#issueco
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-168481572
@nitin-maharana Any bug fix should be against 4.7, any new feature against
master. Bug fixes will be forward merged to master after it is merged to 4.7.
---
If
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-168284288
@remibergsma : Shall I make this one against 4.7. I mean, should I make all
pending PRs against 4.7?
Is it not going to be merged?
---
If your project i
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167953466
LGTM
---
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
ena
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167931759
cc @remibergsma @bhaisaab
---
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 do
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167773312
Thanks a lot @rafaelweingartner , I learned many things.
---
If your project is set up for it, you can reply to this email and have your
reply appear on Git
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167768433
@nitin-maharana now the code is perfect.
I can give a LGTM now.
Sorry if I bothered you with a lot of changes.
---
If your project is set up
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167765606
@rafaelweingartner : Thats cool. I went through the detail of isBlank, but
could not catch. Thank you.
I have updated.
---
If your project is set up for
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167760979
@nitin-maharana now almost everything is ok.
There is only one problem. the
"org.apache.commons.lang.StringUtils.isBlank" also returns true if the valu
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167721727
@rafaelweingartner : I have updated the branch. Can you please review the
change. Thanks.
---
If your project is set up for it, you can reply to this email
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167540899
@nitin-maharana I did not notice that the other PR was to 4.6, I thought it
was to master.
I would just ask you to use a Javadoc block over
âgetVolu
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-167484496
@rafaelweingartner : Thanks for reviewing the change.
The reason to close the old PR is because it was merging with branch 4.6.
I started this PR
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 close
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-166830953
Sure @koushik-das. Thanks.
---
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 do
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-166823449
@nitin-maharana HypervisorUtilsTest test have failed, please do a force
push again.
Code changes LGTM
---
If your project is set up for it, you can reply t
Github user nitin-maharana closed the pull request at:
https://github.com/apache/cloudstack/pull/1206
---
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 fe
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-166556945
Made a new PR https://github.com/apache/cloudstack/pull/1273 with master.
---
If your project is set up for it, you can reply to this email and have your
rep
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1273#issuecomment-166556707
Reference(4.6) : https://github.com/apache/cloudstack/pull/1206
---
If your project is set up for it, you can reply to this email and have your
reply appear
GitHub user nitin-maharana opened a pull request:
https://github.com/apache/cloudstack/pull/1273
CLOUDSTACK-9132: API createVolume takes empty string for name parameter
Steps to Reproduce:
Create a volume using createVolume API where parameter name is empty.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-165757054
cc @koushik-das @kishankavala
---
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 projec
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-165081512
@nitin-maharana that would be my preference. In the UI a pop-up could
explain the consequence of not entering a name.
---
If your project is set up for it, yo
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-165076449
@rafaelweingartner : Thanks for the suggestion. I will follow the same to
write test cases for this.
@DaanHoogland : Should we make the Name field as opti
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164482127
@nitin-maharana I do not agree with you. Take a look at the size of the
method âallocVolumeâ, there are more than 100 lines, that is terrible to
write
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164475760
@DaanHoogland : You mean the name should be an optional field? If that is
the case, we should make the field as optional in UI as well as in API.
@ra
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164417605
Hi @nitin-maharana I agree with @DaanHoogland, we should maintain backward
compatibility. I almost liked you code, I would just suggest you extracting it
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164415638
I don't agree, for two reasons
1. making the field required constitutes a backwards incompatibility of the
API
2. we should use as much reasonable defau
Github user priyankparihar commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164412418
@nitin-maharana Your point looks good. Both(UI and API) should be
consistent.
---
If your project is set up for it, you can reply to this email and have you
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164411409
@DaanHoogland : In UI, we put the field as required field, so user has to
give some name to create a volume. It cannot be null or blank. But according
the ch
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164366344
API with non-empty name:
http://10.102.192.122:8080/client/api?command=createVolume&response=json&name=TempVolume&zoneId=18b1cb1a
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164344691
@DaanHoogland : Yes, your idea also looks good. Previously, it was
generating a random value only in case of NULL. But if we pass an empty string,
it was cre
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1206#issuecomment-164187066
@nitin-maharana I don't agree we should allow the user to get a default
name if (s)he doesn't care. I'd say let the name be the uuid-generated one when
empty (
GitHub user nitin-maharana opened a pull request:
https://github.com/apache/cloudstack/pull/1206
CLOUDSTACK-9132: API createVolume takes empty string for name parameter
Steps to Reproduce:
Create a volume using createVolume API where parameter name is empty.
58 matches
Mail list logo
