Code review comment for lp:~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.

> > 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?

> > 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.

> I think it may make sense to take the time and refractor how we get the
> archives we want the publisher to consider (this might be a good
> opportunity to move some of the soyuz methods out of the registry
> Distribution model, normalize them, and move them into IArchiveSet). The
> end result that I think this could provide us with is easier reuse and
> testing of the logic used to determine which archives needed to be
> considered by the publisher.

+1 but not in this branch. Let's redesign Soyuz one step at a time and keep
the branches limited in focus. You'll appreciate why if you ever want to get
a diff of a revno that fixed a bug.

Also, I really want to get this backend landed before PQM closes at 2200
*today*, then we can let the UI changes bake on edge next cycle.

> For example, how do you test that the
> publisher won't publish archives that have publishing disabled or that are
> deleted?

Yes, this is an architectural problem with that script that needs fixing.

> Lets look at all the different ways we get the archive(s) we want to
> publish depending on the mode of the script:
>
> 1. [distribution.getArchiveByComponent('partner')]
> 2. distribution.getAllPPAs()
> 3. distribution.getPendingPublicationPPAs()
> 4. getUtility(IArchiveSet).getByDistroPurpose(
> distribution, ArchivePurpose.DEBUG)
> 5. getUtility(IArchiveSet).getArchivesForDistribution(
> distribution, purposes=[ArchivePurpose.COPY])
> 6. [distribution.main_archive]

Hmm #4 and #5 look suspiciously similar!

> Do we really want to add to this list? Besides, do we really want to slap
> on yet another method to IArchiveSet - maybe yes, maybe no but I don't
> think so for the former.

I don't see any problem adding to this list. The method I suggested is also
going to be very useful in the future as we add more statuses, which is why I
suggested it.

> Also, should this script be responsible for determining what the publisher
> should do with an archive? In my opinion, I think it might make sense for
> that responsibility to belong to the publisher and for this script to be
> responsible for configuring the publisher based on the settings passed to
> the script and passing the set of archives that we want it to consider.

Neither! It should be a property/behaviour of the archive. But like I said,
let's tackle one thing at a time.

Thanks for the great input though, your suggestions will be very useful.

Let's make the fixes as I suggest and we can land this puppy.

Cheers
J