Re: [Freeipa-devel] Re: [PATCHES] Make plugins use baseldap classes.
Rob Crittenden wrote:
Rob Crittenden wrote:
Pavel Zuna wrote:
Rob Crittenden wrote:
Pavel Zůna wrote:
This is a series of patches that depends on patches:
- Improve attribute printing in the CLI.
- Improve ipalib.plugins.baseldap classes.
All plugins are converted to extend baseldap classes. This makes
things more consistent, fixes some general bugs (with return values
for example) and it also makes plugins easier to maintain (as it
removes a lot of duplicate code).
Because baseldap classes have features that enable us to define
relationships between plugins, I thought it might be best to submit
all of the conversions at once and have all the relationships fully
defined.
Affected plugins:
config
user
host
service
group
hostgroup
netgroup
rolegroup
taskgroup
pwpolicy
There's also a patch that fixes all unit tests.
Jenny, I included you to Cc, because the patch introduces a lot of
changes to the UI (and you're probably not going to like me for this).
Each command extending the LDAP* base classes now has a --raw
option. Without it, data from LDAP is formated and translated to
human readable. For example:
# ipa user-show admin --all
--
user-show:
--
User: admin
user id: admin
full name: Administrator
last name: Administrator
home directory: /home/admin
login shell: /bin/bash
uid number: 999
gid number: 1001
gecos: Administrator
kerberos principal: ad...@pzuna
last password change: 20090904122852Z
password expiration: 20091203122852Z
member of groups: admins
# ipa user-show admin --all --raw
--
user-show:
--
dn: uid=admin,cn=users,cn=accounts,dc=pzuna
uid: admin
cn: Administrator
sn: Administrator
homedirectory: /home/admin
loginshell: /bin/bash
uidnumber: 999
gidnumber: 1001
gecos: Administrator
krbprincipalname: ad...@pzuna
krblastpwdchange: 20090904122852Z
krbpasswordexpiration: 20091203122852Z
memberof: cn=admins,cn=groups,cn=accounts,dc=pzuna
objectclass: top
objectclass: person
objectclass: posixaccount
objectclass: krbprincipalaux
objectclass: inetuser
Advantages: more user friendly, allows for easy localization,
translation of DNs to primary keys (immediately usable as input to
other plugin commands)
I recommend, that you use the --raw flag for testing.
I know it's a lot of changes, so I setup a git repo at:
git://fedorapeople.org/~pzuna/freeipa.git
It should be up-to-date with master and all my patches are applied
there. I hope it makes reviewing them at least a bit easier.
Pavel
Why are you using a pre_callback() to define default values instead
of using default_from? It seems clearer to use that.
You're probably referring to the user plugin, where gecos and
krbprincipalname defaults are set inside pre_callback. It's a residue
from some time ago when we didn't use autofill=True with default_from
and it didn't have any effect on optional parameters. It's a small
change, but I included an updated version of the patch with this email.
Ok. I gather you've moved a lot of logic into the pre_callback plugin
to avoid overriding execute? One other goal we wanted was to allow
plugins to extend other plugins and this may be good step on the way
there. So for example, a user wants to be able to set some extra
objectclass, they could do it with a similar pre_callback extension to
the user plugin (once we do the plumbing for it, that is).
Right. The goal is to have a common execute in the baseclass, that does
most of the dirty work and let the user/plugin author add the specifics
of his plugin in the pre/post callbacks. All the data generated by the
base (before calling python-ldap) is available for modification in the
pre-callbacks and all data coming out of python-ldap is made available
in the post-callback.
And yes, the plugins could be almost endlessly extended this way. For
example, someone could do this:
from ipalib.plugins.user import user_add
class user_add_extended(user_add):
def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
# let the original user_add plugin do its job
super(user_add_extended, self).pre_callback(
ldap, dn, entry_attrs, *keys, **options)
# add an extra object class
entry_attrs['objectclass'].append('new_object_class')
return dn
api.register(user_add_extended)
This also duplicates some values in the attribute_names()
dictionary. I wonder if this can be either created from the
parameters or define a global set somewhere that covers all plugins.
I know, but I couldn't find a better solution. I thought we could add
something like a 'real_name' kwarg to params, but this has 2 main
disadvantages:
1) it only makes sense with parameters that refer to an LDAP attribute
2) it doesn't work for attributes with no param counterparts
The global set is a good idea as long as we consider only our own
plugins. 3rd party
Re: [Freeipa-devel] [PATCH] 277 properly own Apache config files
On Thu, 2009-09-17 at 09:06 -0400, Rob Crittenden wrote: > Martin Nagy wrote: > > On Wed, 2009-09-16 at 13:05 -0400, Rob Crittenden wrote: > >> I goofed on the paths in the original patch I sent on this a while back. > >> This corrects it. > >> > >> I know it looks like we're creating 0-length files here but with the > >> %ghost directive it won't create the files, just own them. > >> > >> rob > > > > Why do you create all the directories and 'touch' the files in the > > %install section when we own them using %ghost? I'm not 100% sure, but I > > believe this isn't required. > > These files are created by ipa-server-install, we don't provide empty > templates, but I don't want IPA to leave orphaned files. > > In order to reference a file in %files, even with %ghost, the file needs > to exist: > http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-files-list-directives.html > > > What is the difference between /etc/ipa/ and /etc/httpd/conf.d/ ? > > /etc/ipa holds configuration files for IPA (the server, admin tools, etc). > > /etc/httpd/conf.d holds the IPA configuration file for Apache. So yes, > we have 2 files named ipa.conf that do completely different things. > > rob Ack. I was looking at the same page but didn't notice the example that actually uses touch, sorry. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 277 properly own Apache config files
Simo Sorce wrote: On Thu, 2009-09-17 at 09:06 -0400, Rob Crittenden wrote: Martin Nagy wrote: On Wed, 2009-09-16 at 13:05 -0400, Rob Crittenden wrote: I goofed on the paths in the original patch I sent on this a while back. This corrects it. I know it looks like we're creating 0-length files here but with the %ghost directive it won't create the files, just own them. rob Why do you create all the directories and 'touch' the files in the %install section when we own them using %ghost? I'm not 100% sure, but I believe this isn't required. These files are created by ipa-server-install, we don't provide empty templates, but I don't want IPA to leave orphaned files. In order to reference a file in %files, even with %ghost, the file needs to exist: http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-files-list-directives.html What is the difference between /etc/ipa/ and /etc/httpd/conf.d/ ? /etc/ipa holds configuration files for IPA (the server, admin tools, etc). /etc/httpd/conf.d holds the IPA configuration file for Apache. So yes, we have 2 files named ipa.conf that do completely different things. Should we rename the second to something like ipa-http.conf ? Simo. We could (in a separate patch) but AFAIK we've never had any confusion over one file vs the other. rob smime.p7s Description: S/MIME Cryptographic Signature ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 277 properly own Apache config files
On Thu, 2009-09-17 at 09:06 -0400, Rob Crittenden wrote: > Martin Nagy wrote: > > On Wed, 2009-09-16 at 13:05 -0400, Rob Crittenden wrote: > >> I goofed on the paths in the original patch I sent on this a while back. > >> This corrects it. > >> > >> I know it looks like we're creating 0-length files here but with the > >> %ghost directive it won't create the files, just own them. > >> > >> rob > > > > Why do you create all the directories and 'touch' the files in the > > %install section when we own them using %ghost? I'm not 100% sure, but I > > believe this isn't required. > > These files are created by ipa-server-install, we don't provide empty > templates, but I don't want IPA to leave orphaned files. > > In order to reference a file in %files, even with %ghost, the file needs > to exist: > http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-files-list-directives.html > > > What is the difference between /etc/ipa/ and /etc/httpd/conf.d/ ? > > /etc/ipa holds configuration files for IPA (the server, admin tools, etc). > > /etc/httpd/conf.d holds the IPA configuration file for Apache. So yes, > we have 2 files named ipa.conf that do completely different things. Should we rename the second to something like ipa-http.conf ? Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 277 properly own Apache config files
Martin Nagy wrote: On Wed, 2009-09-16 at 13:05 -0400, Rob Crittenden wrote: I goofed on the paths in the original patch I sent on this a while back. This corrects it. I know it looks like we're creating 0-length files here but with the %ghost directive it won't create the files, just own them. rob Why do you create all the directories and 'touch' the files in the %install section when we own them using %ghost? I'm not 100% sure, but I believe this isn't required. These files are created by ipa-server-install, we don't provide empty templates, but I don't want IPA to leave orphaned files. In order to reference a file in %files, even with %ghost, the file needs to exist: http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-files-list-directives.html What is the difference between /etc/ipa/ and /etc/httpd/conf.d/ ? /etc/ipa holds configuration files for IPA (the server, admin tools, etc). /etc/httpd/conf.d holds the IPA configuration file for Apache. So yes, we have 2 files named ipa.conf that do completely different things. rob smime.p7s Description: S/MIME Cryptographic Signature ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 275 Fix deprecation warning
On Wed, 2009-09-16 at 13:03 -0400, Rob Crittenden wrote: > This warning was logged in the Apache error log: > > /usr/lib/python2.6/site-packages/mod_python/importer.py:32: > DeprecationWarning: the md5 module is deprecated; use hashlib instead > > Try to import hashlib for md5 and if it fails, fall back to the > deprecated version. Tested on Python 2.4 and 2.6. > > rob Ack. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 277 properly own Apache config files
On Wed, 2009-09-16 at 13:05 -0400, Rob Crittenden wrote: > I goofed on the paths in the original patch I sent on this a while back. > This corrects it. > > I know it looks like we're creating 0-length files here but with the > %ghost directive it won't create the files, just own them. > > rob Why do you create all the directories and 'touch' the files in the %install section when we own them using %ghost? I'm not 100% sure, but I believe this isn't required. What is the difference between /etc/ipa/ and /etc/httpd/conf.d/ ? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
