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
>From dc5f5dd303957697ba26a5590c922a065fe2785b Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Wed, 27 Jul 2011 15:26:57 +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                |   27 +++++++++++++++++++--------
 tests/test_xmlrpc/test_automount_plugin.py |    7 +++++--
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/API.txt b/API.txt
index 42a212b1ebda0f8e1f45b016c5ba421f16ffb24c..cdbf111eb32c950d29a5d1bc9791e7b14e401c3c 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..3c3d44ca3849a93e06b0f767049fe66e063e2b22 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -612,6 +612,7 @@ class automountkey(LDAPObject):
                cli_name='key',
                label=_('Key'),
                doc=_('Automount key name.'),
+               flags=('req_update',),
         ),
         IA5Str('automountinformation',
                cli_name='info',
@@ -850,7 +851,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 +862,28 @@ 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'])
+        key_changed = False
+
+        for key_name, key_update in (('automountinformation', 'newautomountinformation'),
+                                     ('automountkey', 'rename')):
+            if key_update in options:
+                entry_attrs[key_name] = options[key_update]
+                key_changed = True
+
+        if key_changed:
+            key = entry_attrs.get('automountkey', None) \
+                    or options['automountkey']
+            info = entry_attrs.get('automountinformation', None) \
+                    or options.get('automountinformation', None)
+            new_key = options[self.obj.primary_key.name] = self.obj.get_pk(key, info)
+
+            if new_key != keys[-1]: # change the RDN
+                entry_attrs[self.obj.primary_key.name] = new_key
         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))
         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

Reply via email to