-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/21/2011 05:54 PM, Rob Crittenden wrote:
> Jakub Hrozek wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 01/20/2011 11:53 PM, Simo Sorce wrote:
>>> On Thu, 20 Jan 2011 17:27:37 -0500
>>> Dmitri Pal<d...@redhat.com>  wrote:
>>>
>>>> Michael Gregg wrote:
>>>>> Jakub Hrozek wrote:
>>>>> Hi,
>>>>>
>>>>> as discussed in https://bugzilla.redhat.com/show_bug.cgi?id=671019
>>>>> to delete a DNS RR one has to remove its record types one by one.
>>>>>
>>>>> This patch modifies the behaviour so that if the user runs
>>>>> dnsrecord-del<zone>  <record-name>  with no other parameters, the
>>>>> whole record is removed.
>>>>>
>>>>> Alternative solutions might be to expose the internal command that
>>>>> is able to delete the record (although I think it is
>>>>> counterintuitive to have one command to remove record types and one
>>>>> for the whole record) or have a special flag (--del-all?) to remove
>>>>> the whole record.
>>>>>
>>>>> The patch also fixes the unit tests as they didn't reflect all the
>>>>> recent changes.
>>>>
>>>>> Going with this patch sounds good, but to make sure, I polled
>>>>> several
>>>> people here, and they all seemed to think that having to add a
>>>> --del-all or --del-record flag at the end would be better as it would
>>>> be less prone to failure where admins would accidentally delete a
>>>> entire record because they didn't specify anything after the "<zone>
>>>> <record>"
>>>>
>>>>> So, maybe we do need a --del-all or --del-record operator.
>>>>
>>>> Agree.
>>>
>>> +1
>>> Someone may simply push enter accidentally while checking what to write
>>> after the command. It would be rather unfortunate.
>>>
>>> Simo.
>>>
>>>
>>
>> Attached is a new version of the patch that implements --del-all. It
>> also reports failure when deleting a nonexistent RR (new ticket 829).
> 
> nack, this isn't working properly for me.
> 
> Here is how I tested:
> 
> - add a new zone, newzone1
> - ipa dnsrecord-add newzone1 as --a-rec 3.4.5.6
> - ipa dnsrecord-add newzone1 as
>   Record name: as
>   A record: 3.4.5.6
> - ipa dnsrecord-show newzone1 as
>   Record name: as
>   A record: 3.4.5.6
> - ipa dnsrecord-del newzone1 as --del-all
> [ no output ]
> - ipa dnsrecord-show newzone1 as
> ipa: ERROR: as: DNS resource record not found
> 
> So a couple of problems:
> 
> 1. An error should have been thrown when I tried a delete without a
> specific record type.

I agree but I was reluctant to do this because it was perfectly OK to
call "dnsrecord-add" with no options. That would create an empty DNS
record. The interface was orthogonal so "dnsrecord-del" with no options
would remove the record if it was empty. But I don't think an empty DNS
record makes any sense.

I changed the behaviour such that:
* dnsrecord-add with no attributes is no longer allowed. You have to
specify at least one RR type.
* dnsrecord-del with no attributes is no longer allowed. You have to
either specify a RR type or --del-all.

> 2. Some output should be displayed when I delete all records, at least a
> summary.
> 

Agreed and fixed.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk09kfIACgkQHsardTLnvCWyBgCeIos2bWGps/FxL7of6BkuiU8U
AzEAn1Bp/uuoNKB2Qlm2XGGwdDL4dAjl
=I13I
-----END PGP SIGNATURE-----
From f35265821eba70da8984283c5fbd5678a2eccdc3 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Thu, 20 Jan 2011 07:54:14 -0500
Subject: [PATCH] Delete the whole DNS record with no parameters

Error out when deleting a nonexistent DNS record

Also fixes the DNS unit tests.

https://fedorahosted.org/freeipa/ticket/816
https://fedorahosted.org/freeipa/ticket/829
---
 API.txt                              |    3 +-
 ipalib/plugins/dns.py                |   46 +++++++++++++++++++++++++++++++---
 tests/test_xmlrpc/test_dns_plugin.py |   38 +++++++++++++++------------
 3 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/API.txt b/API.txt
index 178f8f5..0a44ad9 100644
--- a/API.txt
+++ b/API.txt
@@ -580,9 +580,10 @@ output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), 'User-friendly
 output: Entry('result', <type 'dict'>, Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('value', <type 'unicode'>, "The primary_key value of the entry, e.g. 'jdoe' for a user")
 command: dnsrecord_del
-args: 2,41,3
+args: 2,42,3
 arg: Str('dnszoneidnsname', cli_name='dnszone', label=Gettext('Zone name', domain='ipa', localedir=None), query=True, required=True)
 arg: Str('idnsname', attribute=True, cli_name='name', label=Gettext('Record name', domain='ipa', localedir=None), multivalue=False, primary_key=True, query=True, required=True)
+option: Flag('del_all', autofill=True, default=False, label=Gettext('Delete all associated records', 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/plugins/dns.py b/ipalib/plugins/dns.py
index af2ba4d..5b5411f 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -380,6 +380,13 @@ class dnsrecord(LDAPObject):
             return self.api.Object[self.parent_object].get_dn(*keys[:-1], **options)
         return super(dnsrecord, self).get_dn(*keys, **options)
 
+    def attr_to_cli(self, attr):
+        try:
+            cliname = attr[:-len('record')].upper()
+        except IndexError:
+            cliname = attr
+        return cliname
+
 api.register(dnsrecord)
 
 
@@ -407,7 +414,9 @@ class dnsrecord_cmd_w_record_options(Command):
                 )
 
     def record_options_2_entry(self, **options):
-        return dict((t, options.get(t, [])) for t in _record_attributes)
+        entries = dict((t, options.get(t, [])) for t in _record_attributes)
+        entries.update(dict((k, []) for (k,v) in entries.iteritems() if v == None ))
+        return entries
 
 
 class dnsrecord_mod_record(LDAPQuery, dnsrecord_cmd_w_record_options):
@@ -415,6 +424,7 @@ class dnsrecord_mod_record(LDAPQuery, dnsrecord_cmd_w_record_options):
     Base class for adding/removing records from DNS resource entries.
     """
     has_output = output.standard_entry
+    no_option_msg = 'No options were provided'
 
     def get_options(self):
         for option in super(dnsrecord_mod_record, self).get_options():
@@ -422,6 +432,14 @@ class dnsrecord_mod_record(LDAPQuery, dnsrecord_cmd_w_record_options):
         for option in self.get_record_options():
             yield option
 
+    def record_options_2_entry(self, **options):
+        entry = super(dnsrecord_mod_record, self).record_options_2_entry(**options)
+        numattr = reduce(lambda x,y: x+y,
+                         map(lambda x: len(x), entry.values()))
+        if numattr == 0:
+            raise errors.OptionError(self.no_option_msg)
+        return entry
+
     def execute(self, *keys, **options):
         ldap = self.obj.backend
 
@@ -456,7 +474,9 @@ class dnsrecord_mod_record(LDAPQuery, dnsrecord_cmd_w_record_options):
         if self.obj.is_pkey_zone_record(*keys):
             entry_attrs[self.obj.primary_key.name] = [u'@']
 
-        self.post_callback(keys, entry_attrs)
+        retval = self.post_callback(keys, entry_attrs)
+        if retval:
+            return retval
 
         return dict(result=entry_attrs, value=keys[-1])
 
@@ -472,6 +492,7 @@ class dnsrecord_add_record(dnsrecord_mod_record):
     Add records to DNS resource.
     """
     NO_CLI = True
+    no_option_msg = 'No options to add a specific record provided.'
 
     def update_old_entry_callback(self, entry_attrs, old_entry_attrs):
         for (a, v) in entry_attrs.iteritems():
@@ -487,6 +508,7 @@ class dnsrecord_add(LDAPCreate, dnsrecord_cmd_w_record_options):
     """
     Add new DNS resource record.
     """
+
     def get_options(self):
         for option in super(dnsrecord_add, self).get_options():
             yield option
@@ -509,6 +531,7 @@ class dnsrecord_delentry(LDAPDelete):
     """
     Delete DNS record entry.
     """
+    msg_summary = _('Deleted record "%(value)s"')
     NO_CLI = True
 
 api.register(dnsrecord_delentry)
@@ -518,6 +541,20 @@ class dnsrecord_del(dnsrecord_mod_record):
     """
     Delete DNS resource record.
     """
+    no_option_msg = 'Neither --del-all nor options to delete a specific record provided.'
+    takes_options = (
+            Flag('del_all',
+                default=False,
+                label=_('Delete all associated records'),
+            ),
+    )
+
+    def execute(self, *keys, **options):
+        if options.get('del_all', False):
+            return self.obj.methods.delentry(*keys)
+
+        return super(dnsrecord_del, self).execute(*keys, **options)
+
     def update_old_entry_callback(self, entry_attrs, old_entry_attrs):
         for (a, v) in entry_attrs.iteritems():
             if not isinstance(v, (list, tuple)):
@@ -526,14 +563,15 @@ class dnsrecord_del(dnsrecord_mod_record):
                 try:
                     old_entry_attrs[a].remove(val)
                 except (KeyError, ValueError):
-                    pass
+                    raise errors.NotFound(reason='%s record with value %s not found' %
+                                          (self.obj.attr_to_cli(a), val))
 
     def post_callback(self, keys, entry_attrs):
         if not self.obj.is_pkey_zone_record(*keys):
             for a in _record_attributes:
                 if a in entry_attrs and entry_attrs[a]:
                     return
-            self.obj.methods.delentry(*keys)
+            return self.obj.methods.delentry(*keys)
 
 api.register(dnsrecord_del)
 
diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py
index 5fb08fd..0be29ff 100644
--- a/tests/test_xmlrpc/test_dns_plugin.py
+++ b/tests/test_xmlrpc/test_dns_plugin.py
@@ -45,7 +45,7 @@ class test_dns(Declarative):
 
     cleanup_commands = [
         ('dnszone_del', [dnszone1], {}),
-        ('dnsrecord_del', [dnszone1, dnsres1], {}),
+        ('dnsrecord_del', [dnszone1, dnsres1], {'del_all' : True}),
     ]
 
     tests = [
@@ -86,7 +86,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -122,7 +123,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -143,7 +145,8 @@ class test_dns(Declarative):
                 'result': {
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [u'5478'],
@@ -157,8 +160,8 @@ class test_dns(Declarative):
 
 
         dict(
-            desc='Search for zones with name server %r' % (u'ns1.%s' % dnszone1),
-            command=('dnszone_find', [], {'idnssoamname': u'ns1.%s' % dnszone1}),
+            desc='Search for zones with name server %r' % (u'ns1.%s.' % dnszone1),
+            command=('dnszone_find', [], {'idnssoamname': u'ns1.%s.' % dnszone1}),
             expected={
                 'summary': None,
                 'count': 1,
@@ -167,7 +170,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [u'5478'],
@@ -200,7 +204,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'FALSE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -233,7 +238,8 @@ class test_dns(Declarative):
                     'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
                     'idnsname': [dnszone1],
                     'idnszoneactive': [u'TRUE'],
-                    'idnssoamname': [u'ns1.%s' % dnszone1],
+                    'nsrecord': [u'ns1.%s.' % dnszone1],
+                    'idnssoamname': [u'ns1.%s.' % dnszone1],
                     'idnssoarname': [u'root.%s.' % dnszone1],
                     'idnssoaserial': [fuzzy_digits],
                     'idnssoarefresh': [fuzzy_digits],
@@ -254,7 +260,7 @@ class test_dns(Declarative):
 
         dict(
             desc='Try to delete non-existent record %r in zone %r' % (dnsres1, dnszone1),
-            command=('dnsrecord_del', [dnszone1, dnsres1], {}),
+            command=('dnsrecord_del', [dnszone1, dnsres1], {'del_all' : True}),
             expected=errors.NotFound(reason='DNS resource record not found'),
         ),
 
@@ -285,6 +291,7 @@ class test_dns(Declarative):
                 'result': [
                     {
                         'dn': u'idnsname=%s,cn=dns,%s' % (dnszone1, api.env.basedn),
+                        'nsrecord': (u'ns1.dnszone.test.',),
                         'idnsname': [u'@'],
                     },
                     {
@@ -329,14 +336,11 @@ class test_dns(Declarative):
 
         dict(
             desc='Delete record %r in zone %r' % (dnsres1, dnszone1),
-            command=('dnsrecord_del', [dnszone1, dnsres1], {}),
+            command=('dnsrecord_del', [dnszone1, dnsres1], {'del_all': True }),
             expected={
                 'value': dnsres1,
-                'summary': None,
-                'result': {
-                    'idnsname': [dnsres1],
-                    'arecord': [u'10.10.0.1'],
-                }
+                'summary': u'Deleted record "%s"' % dnsres1,
+                'result': {'failed': u''},
             },
         ),
 
@@ -347,7 +351,7 @@ class test_dns(Declarative):
             expected={
                 'value': dnszone1,
                 'summary': None,
-                'result': True,
+                'result': {'failed': u''},
             },
         ),
 
-- 
1.7.3.4

Attachment: jhrozek-freeipa-039-03-dns-record-del.patch.sig
Description: PGP signature

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to