Re: [Freeipa-devel] [PATCH] 045 Add DNS record modification command
On Wed, 2011-03-30 at 16:52 -0400, Adam Young wrote: > On 03/30/2011 11:13 AM, Martin Kosek wrote: > > Since this is a new-feature type patch it should be pushed only to master. > > --- > > The DNS record plugin does not support modification of a record. One > > can only add A type addresses to a DNS record or remove the current > > ones. To actually change a DNS record value it has to be removed and > > then added with a desired value. > > > > This patch adds a new DNS plugin command "dnsrecord-mod" which enables > > user to: > > - modify a DNS record value (note than DNS record can hold multiple values > >and those will be overwritten) > > - remove a DNS record when an empty value is passed > > > > New tests for this new command have been added to the CLI test suite. > > > > https://fedorahosted.org/freeipa/ticket/1137 > > > > > > ___ > > Freeipa-devel mailing list > > Freeipa-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/freeipa-devel > > > > NACK, > > The problem is that if there are 10 A records, and I only want to > modify one, I have no way to specify which one. > > The API should be something like: > > ipa dnsrecord-mod ayoung.boston.devel.redhat.com testa 10.10.2.3 > --a-rec=,10.11.12.13 > > > Alternatively, we can decide that we are not going to do mod, and have > the WebUI do a delete and an add: Hm, that may be a valid use-case. We should discuss how we want the DNS record modification to behave. The proposed API is not what we want, since we can modify multiple attributes at once, e.g.: ipa dnsrecord-mod DNSZONE DNSRECORD --a-rec=10.0.0.1 ---rec=::1 I can introduce new option --old--rec for each DNS record type available, e.g. --old-a-rec, --old--rec, --old-srv-rec etc. You would be able to do: ipa dnsrecord-mod DNSZONE DNSRECORD --old-a-rec=10.10.2.3 --a-rec=10.11.12.13 This would of course increase the size of this patch. I tried to find how we treat other multi-value LDAP attributes. In most cases the behavior is the same like in my first patch (user mail, mobile...) or the modification is not supported at all (list of privilege permissions). ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
On Wed, 2011-03-30 at 12:40 -0400, Adam Young wrote: > On 03/28/2011 05:17 PM, Adam Young wrote: > > On 03/28/2011 04:56 PM, Adam Young wrote: > > > To give a little more context: we are llong to split out the > > > logic used to define the views of the entities from the reusable > > > portion of the toolkit. This patch introduces a builder object > > > which contains the temporary state of the entity build process. > > > > > > In the course of writing it, I realized a few things: > > > > > > 1. HBAC and SUDO have two small entities and a single large one. > > > Thus, it makes sense to group them both into a single file per > > > entity. Both hbac.js and sudo.js should shrink more in the future > > > as the custom code gets better refactored and split into reusable > > > components and configuration data. > > > > > > > > > 2. policy.js was a catch all file. Automount will grow > > > significantly this release, and so should have its own file. DNS > > > is complicated enough that it deserves its own top level js file. > > > policy is now reduced to two small entities, both that are very > > > clearly policy. > > > > > > ___ > > > Freeipa-devel mailing list > > > Freeipa-devel@redhat.com > > > https://www.redhat.com/mailman/listinfo/freeipa-devel > > > > > > Self NACK: jsl and unit test errors need to be fixed first. Still > > worth reviewing as is, as fixing that will not change the behavior > > or structure of the end patch. > > > > ___ > > Freeipa-devel mailing list > > Freeipa-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/freeipa-devel > > > > Updated with fix for unit tests. Note that this requires the NACKed > version of > freeipa-admiyo-0216-update-metadata-with-label-changes.patch. The > fixes in the unit tests here resolve the test breakage due to the > metadata updates. Unfortunately the patch failed to apply to the master, there are some conflicts in policy.js. Can you please send a rebased patch? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 759 cache get_ipa_config() output in request context
On Wed, 2011-03-30 at 10:23 -0400, Rob Crittenden wrote: > Some requests generate multiple calls to get_ipa_config(). This patch > caches the return value for this in the request context. > > ticket 1023 > > rob ACK. Tested with user mail & config attribute ipadefaultemaildomain. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 760 don't crash when calculating indirect
On Wed, 2011-03-30 at 10:46 -0400, Rob Crittenden wrote: > Rob Crittenden wrote: > > This prevents an internal error when calculating direct vs indirect > > membership. > > > > ticket 1133 > > > > I accidentally included a change from another patch. Updated patch attached. > > rob I think it is OK. But I would suggest adding some comment to the code - a reason why we pass the ValueError exception. It may not be self-explanatory when we return to this code in the future. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 761 Sort entries on *-find commands
On Wed, 2011-03-30 at 17:14 -0400, Rob Crittenden wrote: > Sort output on find commands based on the baseldap LDAPSearch class. > > A couple tests had to be modified to match the new order. > > ticket 794 > > rob The patch works fine except the case when entries are being added in post_callback. Check this search: ipa permission-find --permissions=write The result is not sorted. I suggest moving the sort process after the self.POST_CALLBACKS calls. What about performance issues? May somebody want to disable the sorting? (e.g. --nosort option). Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
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. 2. There's a IPA.metadata assignment and the like in unit tests, this is redundant. 3. In IPA.entity_builder.section(), the current_section should be added to the current_facet before adding the fields to do incremental construction. 4. In IPA.entity_builder the entity_name can be replaced with entity.name to reduce the number of variables. 5. In IPA.entity_builder the standard_associations() can be replaced standard_association_facets() for consistency. 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. 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. 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. 9. The IPA.entity_builder could be a singleton because it doesn't take any parameters and there's no multi-threading issue. 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. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 21 Escape LDAP characters in member and memberof searches
JR Aquino wrote: On Mar 30, 2011, at 3:03 PM, Rob Crittenden wrote: JR Aquino wrote: On Mar 30, 2011, at 1:01 PM, Stephen Gallagher wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 03/30/2011 03:53 PM, JR Aquino wrote: On Mar 30, 2011, at 12:05 PM, JR Aquino wrote: The FreeIPA framework performs unescaped searches to enumerate group membership. The following patch corrects this behavior. -JR ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Self NACK Attached is the corrected patch. search_group_dn = _ldap_filter.escape_filter_chars(search_group_dn) Is now correctly changed to: search_group_dn = _ldap_filter.escape_filter_chars(group_dn) Nack. This is a step in the right direction, but you're not actually using this value anywhere. I think you wanted to have the next line changed to: searchfilter = "(memberof=%s)" % search_group_dn - -- Stephen Gallagher RHCE 804006346421761 Oh! You are right. Attached is the corrected patch. I don't think you need a new variable for search_group_dn. The value is passed in from a tuple so any changes will be silently lost anyway. Or you can leave it, I think it's probably safer this way (since we can't predict how it will be called in the future), but you should then do the same in get_memberof(). rob I agree with you. For the sake of equality, I have adjusted the patch to address entry_dn with search_entry_dn. ack, pushed to master and ipa-2-0 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] Initial Selenium test cases.
http://www.freeipa.org/page/Selenium -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 759 cache get_ipa_config() output in request context
Martin Kosek wrote: On Wed, 2011-03-30 at 10:23 -0400, Rob Crittenden wrote: Some requests generate multiple calls to get_ipa_config(). This patch caches the return value for this in the request context. ticket 1023 rob ACK. Tested with user mail& config attribute ipadefaultemaildomain. Martin pushed to master and ipa-2-0 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
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. From 849e9ea5afaf79fef8d438eb18d788239bda9207 Mon Sep 17 00:00:00 2001 From: Adam Young Date: Thu, 31 Mar 2011 13:57:35 -0400 Subject: [PATCH] code review fixes --- install/ui/aci.js|8 install/ui/details.js|4 ++-- install/ui/develop.js|2 +- install/ui/dns.js|2 +- install/ui/entity.js | 32 ++-- install/ui/entitylist| 15 +++ install/ui/group.js |2 +- install/ui/host.js |2 +- install/ui/hostgroup.js |2 +- install/ui/netgroup.js |2 +- install/ui/policy.js |2 +- install/ui/service.js|3 ++- install/ui/test/details_tests.js |5 - install/ui/test/entity_tests.js |5 - install/ui/user.js |2 +- 15 files changed, 41 insertions(+), 47 deletions(-) create mode 100644 install/ui/entitylist diff --git a/install/ui/aci.js b/install/ui/aci.js index d51741245f4305a1bdc87acb5fc639b30a5fd23e..7d669fc9155584849ae74f5d0a9d4ed336779509 100644 --- a/install/ui/aci.js +++ b/install/ui/aci.js @@ -60,7 +60,7 @@ IPA.entity_factories.permission = function() { factory:IPA.target_section, label: IPA.messages.objects.permission.target }]). -standard_associations(). +standard_association_facets(). build(); }; @@ -88,7 +88,7 @@ IPA.entity_factories.privilege = function() { add_method: 'add_permission', remove_method: 'remove_permission' }). -standard_associations(). +
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
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@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Including Automount From 849e9ea5afaf79fef8d438eb18d788239bda9207 Mon Sep 17 00:00:00 2001 From: Adam Young Date: Thu, 31 Mar 2011 13:57:35 -0400 Subject: [PATCH] code review fixes --- install/ui/aci.js|8 install/ui/details.js|4 ++-- install/ui/develop.js|2 +- install/ui/dns.js|2 +- install/ui/entity.js | 32 ++-- install/ui/entitylist| 15 +++ install/ui/group.js |2 +- install/ui/host.js |2 +- install/ui/hostgroup.js |2 +- install/ui/netgroup.js |2 +- install/ui/policy.js |2 +- install/ui/service.js|3 ++- install/ui/test/details_tests.js |5 - install/ui/test/entity_tests.js |5 - install/ui/user.js |2 +- 15 files changed, 41 insertions(+), 47 deletions(-) create mode 100644 install/ui/entitylist diff --git a/install/ui/aci.js b/install/ui/aci.js index d51741245f4305a1bdc87acb5fc639b30a5fd23e..7d669fc9155584849ae74f5d0a9d4ed336779509 100644 --- a/install/ui/aci.js +++ b/install/ui/aci.js @@ -60,7 +60,7 @@ IPA.entity_factories.permission = function() { factory:IPA.target_section, label: IPA.messages.objects.permission.target }]). -standard_associations(). +standard_association_facets(). build();
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
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@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Including Automount ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hadn't merged in the changes. Updated patch attached. From bfbb7564e318f596dbaec8e29cdc5fb91600f1eb Mon Sep 17 00:00:00 2001 From: Adam Young Date: Thu, 31 Mar 2011 13:57:35 -0400 Subject: [PATCH] code review fixes --- install/ui/aci.js|8 install/ui/automount.js |2 +- install/ui/details.js|4 ++-- install/ui/develop.js|2 +- install/ui/dns.js|2 +- install/ui/entity.js | 32 ++-- install/ui/entitylist| 15 +++ install/ui/group.js |2 +- install/ui/host.js |2 +- install/ui/hostgroup.js |2 +- install/ui/netgroup.js |2 +- install/ui/policy.js |2 +- install/ui/service.js|3 ++- install/ui/test/details_tests.js |5 - install/ui/test/entity_tests.js |5 - install/ui/user.js |2 +- 16 files changed, 42 insertions(+), 48 deletions(-) create mode 100644 install/ui/entitylist diff --git a/install/ui/aci.js b/install/ui/aci.js index d51741245f4305a1bdc87acb5fc639b30a5fd23e..7d669fc9155584849ae74f5d0a9d4ed336779509 100644 --- a/install/ui/aci.js +++ b
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
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@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Including Automount ___ Freeipa-devel mailing list Freeipa-devel@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@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel This version uses a spec for the details facet, IAW code review feedback From 4dc5d8a71dee4870f6efd6513cfd8e274ebfda4d Mon Sep 17 00:00:00 2001 From: Adam Young Date: Thu, 31 Mar 2011 15:32:29 -0400 Subject: [PATCH] code review fixes this version includes using spec for detail_facets --- install/ui/aci.js| 49 +++-- install/ui/automount.js |8 +++--- install/ui/details.js|4 +- install/ui/develop.js|2 +- install/ui/dns.js|8 +++--- install/ui/entity.js | 47 ++- install/ui/group.js |8 +++--- install/ui/hbac.js | 12 install/ui/host.js | 10 install/ui/hostgroup.js |8 +++--- install/ui/netgroup.js |8 +++--- install/ui/policy.js | 24 ++ install/ui/serverconfig.js |8 -- install/ui/s
[Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
The following patch Removes around 20 lines of code and provides a substantial increase in performance for FreeIPA member/memberof verification searches. The current code base blindly searches static containers for the possible presence of members. This patch provides a method for dynamically identifying the specific objects to verify memberships for. The attached patch addresses ticket: https://fedorahosted.org/freeipa/ticket/1139 ipa hostgroup-find ... - Number of entries returned 52 - real0m20.054s user0m0.934s sys 0m0.050s ipa find-hostgroup ... - Number of entries returned 52 - real0m15.064s user0m0.945s sys 0m0.057s -- Number of entries returned 100 -- real0m16.471s user0m0.814s sys 0m0.040s ipa host-find ... -- Number of entries returned 100 -- real0m41.277s user0m0.806s sys 0m0.060s ipa host-find ... -- Number of entries returned 100 -- real0m16.385s user0m0.814s sys 0m0.053s binWE33diHVCR.bin Description: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch ~ Jr Aquino, GCIH | Information Security Specialist Citrix Online | 7408 Hollister Avenue | Goleta, CA 93117 T: +1 805.690.3478 jr.aqu...@citrixonline.com http://www.citrixonline.com ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 5 Add note about ipa-dns-install to ipa-server-install man page
David O'Brien wrote: Jan Cholasta wrote: On 30.3.2011 01:01, David O'Brien wrote: Jan Cholasta wrote: Added the note so that users know that they can setup DNS at any time after ipa-server-install. https://fedorahosted.org/freeipa/ticket/1082 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel NACK Minor English and style fix: s/ "Note that you can setup DNS at any later time by running ipa-dns-install" / "Note that you can set up a DNS at any time after the initial IPA server install by running ipa-dns-install." Thanks, fixed. cheers ACK Pushed to master and ipa-2-0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
Better formatting for the statistics: -=- ipa hostgroup-find ... - Number of entries returned 52 - real0m20.054s user0m0.934s sys 0m0.050s -=- ipa find-hostgroup ... - Number of entries returned 52 - real0m15.064s user0m0.945s sys 0m0.057s -=- ipa host-find ... -- Number of entries returned 100 -- real0m41.277s user0m0.806s sys 0m0.060s -=- ipa host-find ... -- Number of entries returned 100 -- real0m16.385s user0m0.814s sys 0m0.053s ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
JR Aquino wrote: The following patch Removes around 20 lines of code and provides a substantial increase in performance for FreeIPA member/memberof verification searches. The current code base blindly searches static containers for the possible presence of members. This patch provides a method for dynamically identifying the specific objects to verify memberships for. The attached patch addresses ticket: https://fedorahosted.org/freeipa/ticket/1139 ipa hostgroup-find ... - Number of entries returned 52 - real0m20.054s user0m0.934s sys 0m0.050s ipa find-hostgroup ... - Number of entries returned 52 - real0m15.064s user0m0.945s sys 0m0.057s -- Number of entries returned 100 -- real0m16.471s user0m0.814s sys 0m0.040s ipa host-find ... -- Number of entries returned 100 -- real0m41.277s user0m0.806s sys 0m0.060s ipa host-find ... -- Number of entries returned 100 -- real0m16.385s user0m0.814s sys 0m0.053s There is a typo in the first block, memeber. Wouldn't it be clearer to do a negative test to continue: if not 'member' in r[1]: continue rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
On Mar 31, 2011, at 1:48 PM, Rob Crittenden wrote: > JR Aquino wrote: >> The following patch Removes around 20 lines of code and provides a >> substantial increase in performance for FreeIPA member/memberof verification >> searches. >> >> The current code base blindly searches static containers for the possible >> presence of members. >> >> This patch provides a method for dynamically identifying the specific >> objects to verify memberships for. >> >> The attached patch addresses ticket: >> https://fedorahosted.org/freeipa/ticket/1139 >> >> >> >> ipa hostgroup-find >> >> ... >> >> - >> Number of entries returned 52 >> - >> >> real 0m20.054s >> user 0m0.934s >> sys 0m0.050s >> >> >> ipa find-hostgroup >> >> ... >> >> - >> Number of entries returned 52 >> - >> >> real 0m15.064s >> user 0m0.945s >> sys 0m0.057s >> >> >> -- >> Number of entries returned 100 >> -- >> >> real 0m16.471s >> user 0m0.814s >> sys 0m0.040s >> >> >> ipa host-find >> >> ... >> >> -- >> Number of entries returned 100 >> -- >> >> real 0m41.277s >> user 0m0.806s >> sys 0m0.060s >> >> >> ipa host-find >> >> ... >> >> -- >> Number of entries returned 100 >> -- >> >> real 0m16.385s >> user 0m0.814s >> sys 0m0.053s > > There is a typo in the first block, memeber. > > Wouldn't it be clearer to do a negative test to continue: > > if not 'member' in r[1]: >continue > > rob You're right! Corrected patch attached. binL3jhIuM4kH.bin Description: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative
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@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Including Automount ___ Freeipa-devel mailing list Freeipa-devel@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@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@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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] admiyo-0218-default-all-false.
On 03/30/2011 09:11 PM, Adam Young wrote: Requires patch 217 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel rebased, ACKed in IRC and pushed to master ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] admiyo-0215-Fixed-labels-for-sudo-and-hbac-rules
On 03/29/2011 05:10 AM, Martin Kosek wrote: On Mon, 2011-03-28 at 16:49 -0400, Adam Young wrote: Putting these two patches togetehr because the first changes labels from the server, and the second is only for test data. The second is a separate patch becasue there are other changes from older server side updates. Patch 215: ACK Patch 216: NACK. It breaks the test suite. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Rebased, and ACKed in IRC by edewata. Both were pushed. A different patch fixed the Unit tests. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
To clarify, the high delay times in these stats are due to buffered logging being turned off. The ratio of performance increase is ~the same with buffered logging turned on; e.g 1.9 seconds down to 1.5 On Mar 31, 2011, at 1:43 PM, "JR Aquino" wrote: > Better formatting for the statistics: > > -=- > > > > ipa hostgroup-find > > ... > > - > Number of entries returned 52 > - > > real0m20.054s > user0m0.934s > sys0m0.050s > > -=- > > > ipa find-hostgroup > > ... > > - > Number of entries returned 52 > - > > real0m15.064s > user0m0.945s > sys0m0.057s > > -=- > > > ipa host-find > > ... > > -- > Number of entries returned 100 > -- > > real0m41.277s > user0m0.806s > sys0m0.060s > > -=- > > > ipa host-find > > ... > > -- > Number of entries returned 100 > -- > > real0m16.385s > user0m0.814s > sys0m0.053s > > ___ > Freeipa-devel mailing list > Freeipa-devel@redhat.com > https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Initial Selenium test cases.
On 03/31/2011 02:09 PM, Endi Sukma Dewata wrote: On 3/31/2011 12:06 PM, Endi Sukma Dewata wrote: http://www.freeipa.org/page/Selenium Patch included. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ACK. Pushed to master. Note that I did not review each test in depth, but just spot checked. If we find that we have regular breakages on certain tests, we will investigate those tests more. Please update the wiki page. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel