Merge lp:~nataliabidart/wsgi-oops/subprocess-compress into lp:wsgi-oops

[Natalia Bidart]

[Tim Cole (tcole)]

Looks reasonable; nice style fixes.

review: Approve

[Robert Collins (lifeless)]

You don't seem to check the return code of the compressor process?

review: Abstain

[Nicola Larosa (teknico)]

> Looks reasonable; nice style fixes.

Yes, having the imports in alphabetical order is good; however, I still prefer them on a single line, even against PEP-8. :-)

I would also like to reiterate SteveA's suggestion that gzip is a better CPU/compression tradeoff than bzip2, for text files.

[Rick McBride (rmcbride)]

This looks good, however I'd like to see a response to Robert's question regarding response codes from the compressor process before marking approved.

review: Approve

[lp:~nataliabidart/wsgi-oops/subprocess-compress]

42. By Natalia Bidart on 2009-10-28

Checking return code of bz2 subprocess.

[Natalia Bidart (nataliabidart)]

> You don't seem to check the return code of the compressor process?

Already changed Popen by check_call. Exception handling will be done in #462584.

=== modified file 'canonical/oops/serializer.py'
--- canonical/oops/serializer.py	2009-10-21 21:07:05 +0000
+++ canonical/oops/serializer.py	2009-10-28 13:22:09 +0000
@@ -45,13 +45,15 @@
 """
 
 from datetime import datetime, timedelta
+
+import os
 import pytz
+import re
+import subprocess
+import thread
 import weakref
-import thread
-import re
-import os
 import zipfile
-import bz2
+
 from pprint import pformat
 from canonical.versioninfo import version_info
 
@@ -232,8 +234,12 @@
         self.write_file("version", self._generate_version(oops))
         for entry in oops.get_all():
             self.write_file(entry.entry_name, entry.get_dump_data())
+
+        dump_filename = self._filename
         self.finish_dump()
 
+        return dump_filename
+
     def write_file(self, section, data):
         """Basic writing data to the file system under a directory path"""
         if self._fh:
@@ -463,7 +469,10 @@
 class OOPSBz2Serializer(OOPSRFC822Serializer):
     """A serializer that will generate compressed oops files."""
 
-    def _open_dump_file(self, filename):
-        """Returns a file handler to dump to."""
-        self.bz2_filename = filename + ".bz2"
-        return bz2.BZ2File(self.bz2_filename, "w")
+    def finish_dump(self):
+        # store filename before parent reset it to None
+        filename = self._filename
+        # have the parent finishing the plain oops dump
+        super(OOPSBz2Serializer, self).finish_dump()
+        # compress it
+        subprocess.check_call(('bzip2', filename))

=== modified file 'canonical/oops/tests/test_serializer.py'
--- canonical/oops/tests/test_serializer.py	2009-10-21 19:58:31 +0000
+++ canonical/oops/tests/test_serializer.py	2009-10-28 13:22:09 +0000
@@ -18,11 +18,12 @@
 
 __metaclass__ = type
 
+import bz2
+import logging
+import os
+import rfc822
 import sys
 import unittest
-import rfc822
-import logging
-import bz2
 
 from cStringIO import StringIO
 from canonical.oops.serializer import OOPSRFC822Serializer, OOPSBz2Serializer
@@ -47,7 +48,7 @@
 class OopsSerializerTests(unittest.TestCase):
     """Test oops serialisers"""
     serializer_class = OOPSRFC822Serializer
-    
+
     def _get_result_as_msg(self):
         result = StringIO()
         # Tell the serializer to put data into my own fh
@@ -58,7 +59,7 @@
         result.seek(0)
         # Load the result up as an rfc822 message and close fh
         return rfc822.Message(result), result
-    
+
     def setUp(self):
         """Init this test"""
         self.oops = OOPS( self.get_id )
@@ -146,14 +147,28 @@
 class OopsBz2SerializerTests(OopsSerializerTests):
     """Run the same set of tests with the Zip serializer"""
     serializer_class = OOPSBz2Serializer
+
     def _get_result_as_msg(self):
         # Tell the serializer to dump data to a BZ file
         s = self.serializer_class("key", "/tmp")
         # This is where the serialization is done
-        s.dump(self.oops)
-        fp = bz2.BZ2File(s.bz2_filename)
+        filename = s.dump(self.oops)
+        assert filename is not None
+
+        # store filename to remove it at tearDown time
+        self.compressed_filename = filename + '.bz2'
+
+        self.assertTrue(os.path.exists(self.compressed_filename))
+        self.assertFalse(os.path.exists(filename))
+
+        fp = bz2.BZ2File(self.compressed_filename)
         return rfc822.Message(fp), fp
 
+    def tearDown(self):
+        do_remove = hasattr(self, 'compressed_filename') and \
+                    os.path.exists(self.compressed_filename)
+        if do_remove:
+            os.remove(self.compressed_filename)
 
 if __name__ == '__main__':
     unittest.main()