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