On Thu, 2011-07-28 at 11:49 +0200, Martin Kosek wrote: > On Wed, 2011-07-27 at 14:53 -0400, Rob Crittenden wrote: > > Martin Kosek wrote: > > > On Wed, 2011-07-27 at 12:11 -0400, Dmitri Pal wrote: > > >> On 07/27/2011 12:00 PM, Martin Kosek wrote: > > >>> On Wed, 2011-07-27 at 10:41 -0400, Rob Crittenden wrote: > > >>>> Martin Kosek wrote: > > >>>>> Fix automountkey-mod so that automountkey attribute is correctly > > >>>>> updated. Add this test case to the unit tests. > > >>>>> > > >>>>> https://fedorahosted.org/freeipa/ticket/1528 > > >>>> It fixes the problem but I've found another: --key isn't required so if > > >>>> you don't pass it in then a backtrace will occur: > > >>>> > > >>>> Traceback (most recent call last): > > >>>> File "/home/rcrit/redhat/freeipa-master/ipaserver/rpcserver.py", > > >>>> line > > >>>> 220, in wsgi_execute > > >>>> result = self.Command[name](*args, **options) > > >>>> File "/home/rcrit/redhat/freeipa-master/ipalib/frontend.py", line > > >>>> 425, in __call__ > > >>>> ret = self.run(*args, **options) > > >>>> File "/home/rcrit/redhat/freeipa-master/ipalib/frontend.py", line > > >>>> 731, in run > > >>>> return self.execute(*args, **options) > > >>>> File > > >>>> "/home/rcrit/redhat/freeipa-master/ipalib/plugins/automount.py", > > >>>> line 873, in execute > > >>>> keys += (self.obj.get_pk(options['automountkey'], > > >>>> KeyError: 'automountkey' > > >>>> > > >>>> Also, automountinformation is already required. This may be a leftover > > >>>> from when we used it in description, this can probably be lifted too. > > >>>> > > >>>> rob > > >>> Good catch. I fixed this bug too and I also made --newinfo optional so > > >>> that automountkey may be just renamed without changing its info > > >>> attribute. > > >>> > > >>> I didn't bump up API VERSION as these are either compatible changes or > > >>> they caused server internal error. > > >>> > > >>> Martin > > >> > > >> Should the ticket be moved into 2.1 July sprint then? > > > > > > Yes, I would like this to be included in 2.1. I will move ticket to > > > correct milestone (2.1) if we manage to review&push it before release. > > > > > > Martin > > > > nack. Something is up with _mod. I can't be sure it is this patch or it > > was always here. > > > > In the UI every change wanted to try to rename the entry. On the > > command-line I wasn't able to update the info at all. > > > > rob > > Hm, I think this problem was in the _mod command all the time. > 'description' field was being filled every time which triggered rename > operation. This caused problems. > > I rewrote _mod command so that 'description' (i.e. rename) is filled > only when needed. > > I checked UI and automountkey_mod command worked OK for me. > > Martin
I revisited automountkey_mod command as there was another corner case. get_pk() was changed to include automountinfo in RDN only for direct maps. All tests are OK, even Rob's extended automount test preview. Web UI behaved ok too. Martin
>From c6cb967272aeb85ef3cdecf7ef848c16a87c53fb Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Mon, 1 Aug 2011 16:41:28 +0200 Subject: [PATCH] Fix automountkey-mod Fix automountkey-mod so that automountkey attribute is correctly updated. Add this test case to the unit tests. Make automountkey required for automountkey-mod, otherwise it would cause internal server error. Make --newinfo optional so that automountkey may be just renamed without changing its info attribute. https://fedorahosted.org/freeipa/ticket/1528 --- API.txt | 8 ++-- ipalib/crud.py | 4 ++ ipalib/plugins/automount.py | 53 +++++++++++++++++++--------- tests/test_xmlrpc/test_automount_plugin.py | 7 +++- 4 files changed, 49 insertions(+), 23 deletions(-) diff --git a/API.txt b/API.txt index d78e3529c66e63c7c2e4d6a8676603d3f857ae1a..49b352a4b6995a55cae28361cd7d70205a63f0c2 100644 --- a/API.txt +++ b/API.txt @@ -103,7 +103,7 @@ command: automountkey_add args: 2,7,3 arg: Str('automountlocationcn', cli_name='automountlocation', label=Gettext('Location', domain='ipa', localedir=None), query=True, required=True) arg: IA5Str('automountmapautomountmapname', cli_name='automountmap', label=Gettext('Map', domain='ipa', localedir=None), query=True, required=True) -option: IA5Str('automountkey', attribute=True, cli_name='key', label=Gettext('Key', domain='ipa', localedir=None), multivalue=False, required=True) +option: IA5Str('automountkey', attribute=True, cli_name='key', flags=('req_update',), label=Gettext('Key', domain='ipa', localedir=None), multivalue=False, required=True) option: IA5Str('automountinformation', attribute=True, cli_name='info', label=Gettext('Mount information', domain='ipa', localedir=None), multivalue=False, required=True) option: Str('addattr*', validate_add_attribute, cli_name='addattr', exclude='webui') option: Str('setattr*', validate_set_attribute, cli_name='setattr', exclude='webui') @@ -128,7 +128,7 @@ args: 3,7,4 arg: Str('automountlocationcn', cli_name='automountlocation', label=Gettext('Location', domain='ipa', localedir=None), query=True, required=True) arg: IA5Str('automountmapautomountmapname', cli_name='automountmap', label=Gettext('Map', domain='ipa', localedir=None), query=True, required=True) arg: Str('criteria?', noextrawhitespace=False) -option: IA5Str('automountkey', attribute=True, autofill=False, cli_name='key', label=Gettext('Key', domain='ipa', localedir=None), multivalue=False, query=True, required=False) +option: IA5Str('automountkey', attribute=True, autofill=False, cli_name='key', flags=('req_update',), label=Gettext('Key', domain='ipa', localedir=None), multivalue=False, query=True, required=False) option: IA5Str('automountinformation', attribute=True, autofill=False, cli_name='info', label=Gettext('Mount information', domain='ipa', localedir=None), multivalue=False, query=True, required=False) option: Int('timelimit?', autofill=False, flags=['no_display'], label=Gettext('Time Limit', domain='ipa', localedir=None), minvalue=0) option: Int('sizelimit?', autofill=False, flags=['no_display'], label=Gettext('Size Limit', domain='ipa', localedir=None), minvalue=0) @@ -143,12 +143,12 @@ command: automountkey_mod args: 2,10,3 arg: Str('automountlocationcn', cli_name='automountlocation', label=Gettext('Location', domain='ipa', localedir=None), query=True, required=True) arg: IA5Str('automountmapautomountmapname', cli_name='automountmap', label=Gettext('Map', domain='ipa', localedir=None), query=True, required=True) -option: IA5Str('automountkey', attribute=True, autofill=False, cli_name='key', label=Gettext('Key', domain='ipa', localedir=None), multivalue=False, required=False) +option: IA5Str('automountkey', alwaysask=False, attribute=True, cli_name='key', flags=('req_update',), label=Gettext('Key', domain='ipa', localedir=None), multivalue=False, required=True) option: IA5Str('automountinformation', attribute=True, autofill=False, cli_name='info', label=Gettext('Mount information', domain='ipa', localedir=None), multivalue=False, required=False) option: Str('addattr*', validate_add_attribute, cli_name='addattr', exclude='webui') option: Str('setattr*', validate_set_attribute, cli_name='setattr', exclude='webui') option: Flag('rights', autofill=True, default=False, label=Gettext('Rights', domain='ipa', localedir=None)) -option: IA5Str('newautomountinformation', cli_name='newinfo', label=Gettext('New mount information', domain='ipa', localedir=None)) +option: IA5Str('newautomountinformation?', cli_name='newinfo', label=Gettext('New mount information', domain='ipa', localedir=None)) option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui', flags=['no_output']) option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui', flags=['no_output']) option: Str('version?', exclude='webui', flags=['no_option', 'no_output']) diff --git a/ipalib/crud.py b/ipalib/crud.py index b7a665361804b853da4f00de5ab2b2030c9de86c..97d6430d70f74cacdb7d78d5157a4b1a62ea9405 100644 --- a/ipalib/crud.py +++ b/ipalib/crud.py @@ -190,6 +190,10 @@ class Update(PKQuery): attribute=True, query=True, required=False, autofill=False, alwaysask=True ) + elif 'req_update' in option.flags: + yield option.clone( + attribute=True, required=True, alwaysask=False, + ) else: yield option.clone(attribute=True, required=False, autofill=False) if not self.extra_options_first: diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py index d05e0cf1cb5d91c270c59a79b31efc2a9148a302..ee6e6b8ffc2911b67d3292aeaaba9a2fe0c07707 100644 --- a/ipalib/plugins/automount.py +++ b/ipalib/plugins/automount.py @@ -177,6 +177,7 @@ from ipalib import _, ngettext import ldap as _ldap import os +DIRECT_MAP_KEY = u'/-' class automountlocation(LDAPObject): """ @@ -213,7 +214,7 @@ class automountlocation_add(LDAPCreate): # create auto.master for the new location self.api.Command['automountmap_add'](keys[-1], u'auto.master') self.api.Command['automountmap_add_indirect']( - keys[-1], u'auto.direct', key=u'/-' + keys[-1], u'auto.direct', key=DIRECT_MAP_KEY ) return dn @@ -612,6 +613,7 @@ class automountkey(LDAPObject): cli_name='key', label=_('Key'), doc=_('Automount key name.'), + flags=('req_update',), ), IA5Str('automountinformation', cli_name='info', @@ -714,7 +716,7 @@ class automountkey(LDAPObject): ) def get_pk(self, key, info=None): - if info: + if key == DIRECT_MAP_KEY and info: return self.rdn_separator.join((key,info)) else: return key @@ -727,7 +729,7 @@ class automountkey(LDAPObject): entries = self.methods.find(location, map, automountkey=key)['result'] if len(entries) > 0: - if key == u'/-': + if key == DIRECT_MAP_KEY: info = keykw.get('automountinformation') entries = self.methods.find(location, map, **keykw)['result'] if len(entries) > 0: @@ -756,10 +758,7 @@ class automountkey_add(LDAPCreate): def execute(self, *keys, **options): key = options['automountkey'] info = options.get('automountinformation', None) - if key == '/-': - options[self.obj.primary_key.name] = self.obj.get_pk(key, info) - else: - options[self.obj.primary_key.name] = self.obj.get_pk(key, None) + options[self.obj.primary_key.name] = self.obj.get_pk(key, info) options['add_operation'] = True result = super(automountkey_add, self).execute(*keys, **options) result['value'] = options['automountkey'] @@ -850,7 +849,7 @@ class automountkey_mod(LDAPUpdate): msg_summary = _('Modified automount key "%(value)s"') takes_options = LDAPUpdate.takes_options + ( - IA5Str('newautomountinformation', + IA5Str('newautomountinformation?', cli_name='newinfo', label=_('New mount information'), ), @@ -861,18 +860,38 @@ class automountkey_mod(LDAPUpdate): yield key def pre_callback(self, ldap, dn, entry_attrs, *keys, **options): - entry_attrs['automountinformation'] = options['newautomountinformation'] - entry_attrs['description'] = self.obj.get_pk( - options['automountkey'], - options['newautomountinformation']) + if 'newautomountkey' in options: + entry_attrs['automountkey'] = options['newautomountkey'] + if 'newautomountinformation' in options: + entry_attrs['automountinformation'] = options['newautomountinformation'] return dn def execute(self, *keys, **options): - keys += (self.obj.get_pk(options['automountkey'], - options.get('automountinformation', None)), ) - options[self.obj.primary_key.name] = self.obj.get_pk( - options['automountkey'], - options.get('automountinformation', None)) + ldap = self.api.Backend.ldap2 + key = options['automountkey'] + info = options.get('automountinformation', None) + keys += (self.obj.get_pk(key, info), ) + + # handle RDN changes + if 'rename' in options or 'newautomountinformation' in options: + new_key = options.get('rename', key) + new_info = options.get('newautomountinformation', info) + + if new_key == DIRECT_MAP_KEY and not new_info: + # automountinformation attribute of existing LDAP object needs + # to be retrieved so that RDN can be generated + dn = self.obj.get_dn(*keys, **options) + (dn_, entry_attrs_) = ldap.get_entry(dn, ['automountinformation']) + new_info = entry_attrs_.get('automountinformation', [])[0] + + # automounkey attribute cannot be overwritten so that get_dn() + # still works right + options['newautomountkey'] = new_key + + new_rdn = self.obj.get_pk(new_key, new_info) + if new_rdn != keys[-1]: + options['rename'] = new_rdn + result = super(automountkey_mod, self).execute(*keys, **options) result['value'] = options['automountkey'] return result diff --git a/tests/test_xmlrpc/test_automount_plugin.py b/tests/test_xmlrpc/test_automount_plugin.py index 0face2ef0d28823c08bf073f645f7bd2931c3a7d..cfea61270fca423fe29e00747b507d170e05d255 100644 --- a/tests/test_xmlrpc/test_automount_plugin.py +++ b/tests/test_xmlrpc/test_automount_plugin.py @@ -34,6 +34,7 @@ class test_automount(XMLRPC_test): locname = u'testlocation' mapname = u'testmap' keyname = u'testkey' + keyname_rename = u'testkey_rename' keyname2 = u'testkey2' description = u'description of map' info = u'ro' @@ -127,9 +128,11 @@ class test_automount(XMLRPC_test): Test the `xmlrpc.automountkey_mod` method. """ self.key_kw['newautomountinformation'] = self.newinfo + self.key_kw['rename'] = self.keyname_rename res = api.Command['automountkey_mod'](self.locname, self.mapname, **self.key_kw)['result'] assert res - assert_attr_equal(res, 'automountinformation', 'rw') + assert_attr_equal(res, 'automountinformation', self.newinfo) + assert_attr_equal(res, 'automountkey', self.keyname_rename) def test_a_automountmap_mod(self): """ @@ -144,7 +147,7 @@ class test_automount(XMLRPC_test): """ Test the `xmlrpc.automountkey_del` method. """ - delkey_kw={'automountkey': self.keyname, 'automountinformation' : self.newinfo, 'raw': True} + delkey_kw={'automountkey': self.keyname_rename, 'automountinformation' : self.newinfo, 'raw': True} res = api.Command['automountkey_del'](self.locname, self.mapname, **delkey_kw)['result'] assert res assert_attr_equal(res, 'failed', '') -- 1.7.6
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
