Merge lp:~stub/launchpad/librarian-gc into lp:launchpad

Per Bug #437084, the librarian garbage collector is still failing on production. This is the fix.

Now with test case.

> Per Bug #437084, the librarian garbage collector is still failing on
> production. This is the fix.
>
> Now with test case.

Thanks stub - nicely tested.

r=me (Teeny style-guide questions below.)

> === modified file 'lib/canonical/librarian/ftests/test_gc.py'
> --- lib/canonical/librarian/ftests/test_gc.py 2009-09-26 09:06:13 +0000
> +++ lib/canonical/librarian/ftests/test_gc.py 2009-10-01 07:09:13 +0000
> @@ -15,6 +15,7 @@
> from pytz import utc
> from sqlobject import SQLObjectNotFound
> from storm.locals import SQL, AutoReload
> +import transaction
> from zope.component import getUtility
>
> from canonical.config import config
> @@ -560,6 +561,21 @@
> self.assert_(os.path.exists(noisefile2_path))
> self.assert_(os.path.exists(noisefile3_path))
>
> + def test_deleteUnwantedFilesBug437084(self):

Just looking at https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
should this be:

       def test_deleteUnwantedFiles_bug437084(self):

or similar?

> + # There was a bug where delete_unwanted_files() would die
> + # if the last file found on disk was unwanted.
> + self.layer.switchDbUser(dbuser='testadmin')
> + content='foo'
> + self.client.addFile(
> + 'foo.txt', len(content), StringIO(content), 'text/plain',
> + )

I always get confused on this one, but I think you've used the tuple
style here, rather than the multiline function call style? That is,
I the closing ')' should be on the previous line and without the extra
comma? See

https://dev.launchpad.net/PythonStyleGuide#Multiline function calls

> + # Roll back the database changes, leaving the file on disk.
> + transaction.abort()

Nice!

> +
> + self.layer.switchDbUser(config.librarian_gc.dbuser)
> +
> + # This should cope.
> + librariangc.delete_unwanted_files(self.con)
>
> def test_cronscript(self):
> script_path = os.path.join(
>
> === modified file 'lib/canonical/librarian/librariangc.py'
> --- lib/canonical/librarian/librariangc.py 2009-09-26 08:43:07 +0000
> +++ lib/canonical/librarian/librariangc.py 2009-10-01 07:09:13 +0000
> @@ -578,6 +578,7 @@
> next_wanted_content_id = get_next_wanted_content_id()
>
> if (config.librarian_server.upstream_host is None
> + and next_wanted_content_id is not None
> and next_wanted_content_id < content_id):
> log.error(
> "LibraryFileContent %d exists in the database but "

And I asked:

<noodles775> stub: hi! Just looking at your MP... would it be worth in addition creating a separate condition to log a warning for this situation where the file exists on disk but not in the db?
<stub> That is actually normal - client.addFile(foo,...); transaction.abort() leaves a file on disk
<noodles775> OK - I assumed that the transaction is only aborted in abnormal circumstances.
<stub> Nah - anytime a web request needs to be retried after uploading a file for instance.
<noodles775> k. great.

--
Michael

> > + def test_deleteUnwantedFilesBug437084(self):
>
> Just looking at https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
> should this be:
>
> def test_deleteUnwantedFiles_bug437084(self):
>
> or similar?

As per IRC, test_delete_unwanted_files_bug437084().

> > + # There was a bug where delete_unwanted_files() would die
> > + # if the last file found on disk was unwanted.
> > + self.layer.switchDbUser(dbuser='testadmin')
> > + content='foo'
> > + self.client.addFile(
> > + 'foo.txt', len(content), StringIO(content), 'text/plain',
> > + )
>
> I always get confused on this one, but I think you've used the tuple
> style here, rather than the multiline function call style? That is,
> I the closing ')' should be on the previous line and without the extra
> comma? See
>
> https://dev.launchpad.net/PythonStyleGuide#Multiline function calls

Yup. Copy and paste glitch. Fixed.

Thanks :)