[Freeipa-devel] [PATCH] 039 Delete the whole DNS record with no parameters

[Simo Sorce]

On Mon, 2011-01-24 at 15:51 +0100, Jakub Hrozek wrote:
> -----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<dpal at 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.

Apparently this is not effective, I was able to add an empty DNS
record. 

> * dnsrecord-del with no attributes is no longer allowed. You have to
> either specify a RR type or --del-all.

This one tested right.

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

This also checks out.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York