cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ough <alex.o...@sungard.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Tue, 11 Mar 2014 17:43:14 GMT
I worked on a couple of items, so can you give me the confirmation for the
rest items so that I can wrap this up?
I really want to include this into 4.4.

Thanks
Alex Ough


On Mon, Mar 10, 2014 at 3:41 PM, Alex Ough <alex.ough@sungard.com> wrote:

> Please see my reply/question in blue.
>
> Thanks
> Alex Ough
>
>
> On Mon, Mar 10, 2014 at 1:25 PM, Chiradeep Vittal <
> Chiradeep.Vittal@citrix.com> wrote:
>
>> I havenĀ¹t looked at it because it is huge. But preliminary scan:
>>
>> - there are unit tests missing for changes to core CS code
>>
>          Unit tests for only new classes were added because I couldn't
> find unit tests of the ones I modified
>
>
>> - uses com.amazonaws.util.json (why?)
>>
>          They are used to handle the json objects. Let me know if you
> want me to replace it with another.
>
> - code format does not follow coding convention ( placement of {}, camel
>> case api_interface )
>>
>          Done
>
>
>> - package namespace is com.cloud instead of org.apache for new files
>>
>          I didn't know that. So do I need to use 'org.apache' package for
> all new classes?
>
> - no file-level comments. What does LocalAccountManager do? Why does it
>> exist? Etc.
>>
>          Done.
>
>
>> - No interfaces, only abstract classes. No use of @Inject. While this
>> might be OK, it does make it harder to test and does not follow the rest
>> of ACS conventions.
>>
>         I don't think there is any interface, but let me know if you find
> any.
>         Any recommendation to replace @inject?
>
>>
>> I would urge the submitter to break up the submission.
>> A) the changes to CS core, with explanations / comments and tests
>> B) the sync service should be an interface with concrete implementations
>> in its own package
>> C) more tests
>>
>         can you give me a little specific what kind of tests you need
> more?
>
>
>
>>  On 3/10/14, 3:37 AM, "Daan Hoogland" <daan.hoogland@gmail.com> wrote:
>>
>> >Hi everyody,
>> >
>> >The people at sungard have been making this change for 4.4 and I want
>> >to merge/apply it this week. It is more then a screenfull and might
>> >cause issue is a setup or two.
>> >
>> >have a look at https://reviews.apache.org/r/17790/
>> >
>> >a ref to ticket and fs page are in the review request.
>> >
>> >kind regards,
>> >--
>> >Daan
>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message