[Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative

Adam Young ayoung at redhat.com
Thu Mar 31 21:30:37 UTC 2011


On 03/31/2011 04:29 PM, Adam Young wrote:
> On 03/31/2011 03:27 PM, Adam Young wrote:
>> On 03/31/2011 03:12 PM, Adam Young wrote:
>>> On 03/31/2011 02:09 PM, Adam Young wrote:
>>>> On 03/31/2011 12:51 PM, Endi Sukma Dewata wrote:
>>>>> On 3/31/2011 8:43 AM, Adam Young wrote:
>>>>>> rebased both patches
>>>>>
>>>>> I don't see any code change in the rebased patches, only new 
>>>>> commit ID's, I hope this is correct.
>>>>>
>>>>> Some comments (some of which had been discussed over IRC):
>>>>>
>>>>> 1. I ran my Selenium test cases against 215, 216, and 217 together,
>>>>>    so far there's no failure.
>>>> Good to know.
>>>>
>>>>>
>>>>> 2. There's a IPA.metadata assignment and the like in unit tests, this
>>>>>    is redundant.
>>>>
>>>> yeah, I suspect it was from before I explicitly set async.  removed.
>>>>
>>>>>
>>>>> 3. In IPA.entity_builder.section(), the current_section should be 
>>>>> added
>>>>>    to the current_facet before adding the fields to do incremental
>>>>>    construction.
>>>> Done.
>>>>
>>>>>
>>>>> 4. In IPA.entity_builder the entity_name can be replaced with
>>>>>    entity.name to reduce the number of variables.
>>>>
>>>> Done
>>>>>
>>>>> 5. In IPA.entity_builder the standard_associations() can be replaced
>>>>>    standard_association_facets() for consistency.
>>>> Done
>>>>>
>>>>> 6. In the permission entity definition, the 'add_fields' is used
>>>>>    inconsistently to add a section (i.e. IPA.target_section). The
>>>>>    solution is either adding 'add_sections' or converting
>>>>>    IPA.target_sections into widgets. I think adding 'add_sections' is
>>>>>    simpler because widgets is designed to represent a single 
>>>>> attribute.
>>>>
>>>> Gonna punt on this for this patch.  Not certain on the correct 
>>>> approach, either to make adders have sections, or to convert the 
>>>> one custom section we have to a widget.  Either way, beyond the 
>>>> scope of this patch, and it will only affect one entity and the 
>>>> builder when we decide.
>>>>
>>>>>
>>>>> 7. The IPA.entity_builder.details_facet() takes an array of sections
>>>>>    instead of a spec object. This limits the expandability of the
>>>>>    builder interface. It should take a spec object with a 'sections'
>>>>>    attribute containing the array of sections, this would be 
>>>>> consistent
>>>>>    with the other interfaces.
>>>>
>>>> I thought about it, but there is nothing that we want to customize 
>>>> in the details_facet.  We can always do a custom facet to 
>>>> customize.  An alternative is to check the type of that is passed 
>>>> in to the details_section method on the builder, check if it is an 
>>>> object or an array, and treat it like a spec or array depending.
>>>>
>>>>>
>>>>> 8. In IPA.entity_builder.search_facet(), there's no need to call
>>>>>    current_facet.init() because all facets will be initialized by
>>>>>    the entity when IPA.start_entities() is invoked.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> 9. The IPA.entity_builder could be a singleton because it doesn't 
>>>>> take
>>>>>    any parameters and there's no multi-threading issue.
>>>> Going to leave it like this for now.  That change would be limited 
>>>> to a single file (entity.js) and can be done if we decide we want 
>>>> to with a very small patch.
>>>>
>>>>
>>>>>
>>>>> 10. In IPA.details_refresh() it calls IPA.refresh_devel_hook()
>>>>>     to execute a code specifically used by testing. I think ideally
>>>>>     the develop.js should modify the entity_builder instance to 
>>>>> generate
>>>>>     details facet with test-specific code. This requires item #9. But
>>>>>     for now at least we should rename IPA.refresh_devel_hook() into
>>>>>     IPA.details_refresh_devel_hook() for clarity.
>>>>
>>>> Good idea.  Done
>>>>
>>>>
>>>>  I had already submitted patch 
>>>> freeipa-admiyo-0218-default-all-false.  Some of the fixes here 
>>>> would conflict with that patch if rebased.  What I've attached is 
>>>> on top of 217-2 and 218-1.
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Freeipa-devel mailing list
>>>> Freeipa-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>> Including Automount
>>>
>>>
>>> _______________________________________________
>>> Freeipa-devel mailing list
>>> Freeipa-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>> Hadn't merged in the changes.  Updated patch attached.
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> This version uses a spec for the details facet, IAW code review feedback
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
ACKed after lengthy discussion and minor fix up in IRC

Patches 217 and 219 pushed to master
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/7ae82a8d/attachment.htm>


More information about the Freeipa-devel mailing list