Code review comment for lp://staging/~cody-somerville/launchpad/ppa-deletion

> On Friday 26 March 2010 01:15:27 Cody A.W. Somerville wrote:
> > > Doing more than necessary makes for a fragile test if other parts of the
> > > system change. A better test would be to throw some random files into
> > > the root directory of the archive and then assert that they get deleted
> > > when you call deleteArchive, along with the status change.
> >
> > You bring up a good point. However, I think what we need to do to
> > successfully test our ability to delete an archive and not just our ability
> > to delete a directory with some files in it is to attempt deleting a simple
> > but typical archive. If there is higher level way of achieving the same
> > result instead of calling those publisher methods manually then I think
> > that would be a good compromise. What do you think?
>
> In the nicest possible way, I think it's crazy.
>
> The code you're testing doesn't care what you're deleting, it just does a
> shutil.rmtree(). It's totally pointless putting any kind of data structure in
> there because you're not using it; it just wastes test suite time. If we were
> testing deleting of only certain parts of the archive, then of course it would
> be correct to set up a real archive. But we're not.
>
> Seriously, just poke some files in the archive root and test that they get
> removed.

Okay.

> > > This code has got a subtle bug - it will only process archives marked
> > > with publishing enabled because of the filter just above the context of
> > > the diff.
> >
> > This was actually an intentional decision. Deleting an archive is a
> > function of the publisher. If the publisher is disabled for an archive, I
> > don't think we should process requests to delete the archive just like we
> > wouldn't process pending publications or requests to copy a package into
> > the archive.
> >
> > You're welcome to disagree on me this. I suspect that you might reply with
> > that the flag's actual intention is to 'disable publishing for the
> > archive', not 'disable the publisher for that archive'. However, I think
>
> Correct :)
>
> > even if this is the case then we should consider re-purposing it to
> > include this slightly broader scope of functionality. If you're interested
> > in my rationale or some potential use cases for this, I'd be happy to
> > discuss it further with you on IRC.
> >
> > However, looking at your UI branch, it seems that you disable the PPA as
> > well as set the status to ArchiveStatus.DELETING which of course puts a
> > 'wrench' in this plan. See below for further thoughts related to this.
>
> Think about this from the user's point of view (and the user's PoV should
> drive everything). They're clicking a button that says to delete the PPA.
> This is an action that has a certain amount of finality about it! Having to
> then go and discover that it didn't really delete because you forgot to tick a
> "publish" checkbox on the +edit page is rather weird, don't you think?

I don't feel strongly about this. However, the publish bool is not exposed to users (or on the form at all for anyone IIRC) so they'd only run into this if an admin intentionally disabled publishing for an archive. This I would consider a feature that might be used in circumstances that its critical that the publisher not modify an archive in any way for some reason. Furthermore, if we did do it this way then we'd probably warn the user when trying to delete a PPA that has publishing disabled.

In the mean time, I'll implement this as you request.

> > > What do you think to adding IArchiveSet.getByStatus()? Then at the end
> > > of the publisher script we can grab a list of archives that are in the
> > > DELETING state and simply call publisher.deleteArchive(). The new method
> > > on IArchiveSet would require a simple new test.
> >
> > Could you elaborate on what exactly you're referring to? I think there is a
> > lot that could be improved in this script (its a bit of a hack!).
>
> The script is gross, but I don't want to make wholesale improvements to it in
> this branch, let's keep focused (although drive-by fixes are great and
> encouraged).
>
> > However, you've made me notice a glaring bug. Once an archive's status is
> > set to ArchiveStatus.DELETED it'll start getting published again. However,
> > because PPAs are filtered by those with pending publications it means that
> > deleted PPAs won't get published unless it somehow gets a pending
> > publication (which we don't necessarily do anything to actively prevent and
> > I wouldn't trust that such a scenario wouldn't somehow occur).
>
> Actually we do - disabling the PPA prevents uploads and copying.
>
> However, as well as setting DELETED, you should set publish=False to be on the
> safe side. This will also act as a cleanup marker when we fold these booleans
> into the status enum.

Done.

Cheers,

Cody