cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alena Prokharchyk <Alena.Prokharc...@citrix.com>
Subject Re: Review Request 17591: CLOUDSTACK-5872: Async response from addAccountToProject doesn't contain useful information
Date Tue, 04 Mar 2014 18:57:32 GMT
Daan, I don’t see the need for doing that. Take deleteAccountFromProject
API. Every delete command in CloudStack returns the success response;
seeing true/false is more than enough to figure out if the deletion was
successful. We follow this format in every delete* command in CS.

Now take addAccountToProject. This command neither modifies the account,
nor the project. It just creates a relationship between project and the
account, that you can later get by executing listProjectAccounts API.
Although we might have return success/accountObject-ProjectId in the
response instead of just returning the success, I don’t see the big need
for that. But if you insist on fixing it anyway, yes, the right way would
be extending SuccessResponse and use it in this particular API. Although
the response name “success” that we have to preserve, would be a bit
misleading considering that we are gonna return other information besides
just boolean param.


-Alena.


On 3/4/14, 10:48 AM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:

>I mean not returning a SuccessResponse but some custom variety to it.
>
>On Tue, Mar 4, 2014 at 7:44 PM, Alena Prokharchyk
><Alena.Prokharchyk@citrix.com> wrote:
>> We are not gonna change the response for API calls mentioned in the bug
>> filed by David. We will just leave Apis the way it is. The original bug
>> should be closed as WontFix, and I already reverted the commit.
>>
>> I didn't quite get your question about other APIs, could you please
>> elaborate?
>>
>> -Alena.
>>
>> On 3/4/14, 10:28 AM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:
>>
>>>We can not change the response for this api call? so other apis aren't
>>>touched?
>>>
>>>On Tue, Mar 4, 2014 at 6:31 PM, Alena Prokharchyk
>>><Alena.Prokharchyk@citrix.com> wrote:
>>>> Daan, you can¹t extend the response with more parameters as its a
>>>>generic
>>>> SuccessResponse used by many Apis, and you can¹t add anything a
>>>>particular
>>>> call specific to that.
>>>>
>>>> I think we shouldn¹t fix the bug at this point. You can always get the
>>>> additional information you need by executing corresponding list* call.
>>>>
>>>> -Alena.
>>>>
>>>> On 3/4/14, 12:05 AM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:
>>>>
>>>>>H Alena,
>>>>>
>>>>>You are right. I was under the impression that response format was
>>>>>being extended.
>>>>>
>>>>>I saw you already reverted. If the response was extended with the
>>>>>extra value it would be alright, would it?
>>>>>
>>>>>On Mon, Mar 3, 2014 at 7:06 PM, Alena Prokharchyk
>>>>><Alena.Prokharchyk@citrix.com> wrote:
>>>>>> Daan, this fix break API compatibility! All the customers using
>>>>>>these
>>>>>>API,
>>>>>> will end up with broken code on their side. As the response format
>>>>>>is
>>>>>> changed. Can you please roll it back?
>>>>>>
>>>>>> Thanks,
>>>>>> Alena.
>>>>>>
>>>>>> On 3/3/14, 2:03 AM, "daan Hoogland" <daan.hoogland@gmail.com>
wrote:
>>>>>>
>>>>>>>
>>>>>>>-----------------------------------------------------------
>>>>>>>This is an automatically generated e-mail. To reply, visit:
>>>>>>>https://reviews.apache.org/r/17591/#review35953
>>>>>>>-----------------------------------------------------------
>>>>>>>
>>>>>>>Ship it!
>>>>>>>
>>>>>>>
>>>>>>>ebcaec8632dbd92c071317f3190915244a287afb
>>>>>>>
>>>>>>>- daan Hoogland
>>>>>>>
>>>>>>>
>>>>>>>On Jan. 31, 2014, 2:51 p.m., David Grizzanti wrote:
>>>>>>>>
>>>>>>>> -----------------------------------------------------------
>>>>>>>> This is an automatically generated e-mail. To reply, visit:
>>>>>>>> https://reviews.apache.org/r/17591/
>>>>>>>> -----------------------------------------------------------
>>>>>>>>
>>>>>>>> (Updated Jan. 31, 2014, 2:51 p.m.)
>>>>>>>>
>>>>>>>>
>>>>>>>> Review request for cloudstack.
>>>>>>>>
>>>>>>>>
>>>>>>>> Bugs: CLOUDSTACK-5872
>>>>>>>>     https://issues.apache.org/jira/browse/CLOUDSTACK-5872
>>>>>>>>
>>>>>>>>
>>>>>>>> Repository: cloudstack-git
>>>>>>>>
>>>>>>>>
>>>>>>>> Description
>>>>>>>> -------
>>>>>>>>
>>>>>>>> CLOUDSTACK-5872: Async response from addAccountToProject
doesn't
>>>>>>>>contain useful information
>>>>>>>>
>>>>>>>> Updated the following classes to return a project object
after
>>>>>>>>async
>>>>>>>>jobs complete:
>>>>>>>>  api/src/com/cloud/projects/ProjectService.java     |  6
++--
>>>>>>>>  .../user/account/AddAccountToProjectCmd.java       |  7
+++--
>>>>>>>>  .../user/account/DeleteAccountFromProjectCmd.java  |  7
+++--
>>>>>>>>  .../user/project/UpdateProjectInvitationCmd.java   |  8
+++--
>>>>>>>>  server/src/com/cloud/projects/ProjectManager.java  |  2
+-
>>>>>>>>  .../src/com/cloud/projects/ProjectManagerImpl.java | 34
>>>>>>>>+++++++++++-----------
>>>>>>>>  .../com/cloud/projects/MockProjectManagerImpl.java | 16
>>>>>>>>+++++-----
>>>>>>>>
>>>>>>>> Previously these API commands only returned "success =>
true" in
>>>>>>>>the
>>>>>>>>aysnc job result.  Now it returns the project that a user
was
>>>>>>>>added/deleted to.
>>>>>>>>
>>>>>>>>
>>>>>>>> Diffs
>>>>>>>> -----
>>>>>>>>
>>>>>>>>   api/src/com/cloud/projects/ProjectService.java dc882ef
>>>>>>>>
>>>>>>>>api/src/org/apache/cloudstack/api/command/user/account/AddAccountTo
>>>>>>>>Pr
>>>>>>>>oj
>>>>>>>>ec
>>>>>>>>tCmd.java 36df579
>>>>>>>>
>>>>>>>>api/src/org/apache/cloudstack/api/command/user/account/DeleteAccoun
>>>>>>>>tF
>>>>>>>>ro
>>>>>>>>mP
>>>>>>>>rojectCmd.java f6aa36c
>>>>>>>>
>>>>>>>>api/src/org/apache/cloudstack/api/command/user/project/UpdateProjec
>>>>>>>>tI
>>>>>>>>nv
>>>>>>>>it
>>>>>>>>ationCmd.java dda7b54
>>>>>>>>   server/src/com/cloud/projects/ProjectManager.java f568146
>>>>>>>>   server/src/com/cloud/projects/ProjectManagerImpl.java 5a0ed1c
>>>>>>>>   server/test/com/cloud/projects/MockProjectManagerImpl.java
>>>>>>>>dc377ff
>>>>>>>>
>>>>>>>> Diff: https://reviews.apache.org/r/17591/diff/
>>>>>>>>
>>>>>>>>
>>>>>>>> Testing
>>>>>>>> -------
>>>>>>>>
>>>>>>>> Testing done on master.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> David Grizzanti
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>--
>>>>>Daan
>>>>
>>>
>>>
>>>
>>>--
>>>Daan
>>
>
>
>
>-- 
>Daan

Mime
View raw message