launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-723733 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #723733 Clear LC_TIME in ec2 update-image
  https://bugs.launchpad.net/bugs/723733

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-723733/+merge/50935

= Summary =

To update the AMI image, as per https://dev.launchpad.net/EC2Test/Image, it's best to set LC_TIME to some known value because some users (including at least Julian) have run into problems when their personal settings weren't handled well by the image.  This makes for an ugly workaround.


== Proposed fix ==

The code for "ec2 update-image" already clears the LC_ALL and LANG variables for the same reason.  It should drop LC_TIME as well.


== Pre-implementation notes ==

Julian agrees.


== Implementation details ==

I also encapsulated various pdb.set_trace() calls into a single method.  Each of those calls was reported as lint, and now there's only one left.  Not perfect, but perhaps better than suppressing warnings and repeating the same iffy pattern all over the place.

The function's name looks a little unusual, but I think best says "call set_trace if this variable is True" without requiring the reader to look up what it does.


== Tests ==

Ahem.  All I can think of is: run the image.


== Demo and Q/A ==

Run the "ec2 update-image" command line from https://dev.launchpad.net/EC2Test/Image but with LC_TIME set to en_GB instead of en_US.UTF-8.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/devscripts/ec2test/builtins.py

./lib/devscripts/ec2test/builtins.py
     153: Line contains a call to pdb.
-- 
https://code.launchpad.net/~jtv/launchpad/bug-723733/+merge/50935
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-723733 into lp:launchpad.

=== modified file 'lib/devscripts/ec2test/builtins.py'
--- lib/devscripts/ec2test/builtins.py	2010-11-29 21:32:06 +0000
+++ lib/devscripts/ec2test/builtins.py	2011-02-23 14:49:57 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The command classes for the 'ec2' utility."""
@@ -147,6 +147,12 @@
     return filename
 
 
+def set_trace_if(enable_debugger=False):
+    """If `enable_debugger` is True, drop into the debugger."""
+    if enable_debugger:
+        pdb.set_trace()
+
+
 class EC2Command(Command):
     """Subclass of `Command` that customizes usage to say 'ec2' not 'bzr'.
 
@@ -272,8 +278,7 @@
             pqm_submit_location=None, pqm_email=None, postmortem=False,
             attached=False, debug=False, open_browser=False,
             include_download_cache_changes=False):
-        if debug:
-            pdb.set_trace()
+        set_trace_if(debug)
         if branch is None:
             branch = []
         branches, test_branch = _get_branches_and_test_branch(
@@ -387,8 +392,7 @@
                 "wide because this will break the rest of Launchpad.\n\n"
                 "***************************************************\n")
             raise
-        if debug:
-            pdb.set_trace()
+        set_trace_if(debug)
         if print_commit and dry_run:
             raise BzrCommandError(
                 "Cannot specify --print-commit and --dry-run.")
@@ -482,8 +486,7 @@
     def run(self, test_branch=None, branch=None, trunk=False, machine=None,
             instance_type=DEFAULT_INSTANCE_TYPE, debug=False,
             include_download_cache_changes=False, demo=None):
-        if debug:
-            pdb.set_trace()
+        set_trace_if(debug)
         if branch is None:
             branch = []
         branches, test_branch = _get_branches_and_test_branch(
@@ -558,8 +561,7 @@
     def run(self, ami_name, machine=None, instance_type='m1.large',
             debug=False, postmortem=False, extra_update_image_command=None,
             public=False):
-        if debug:
-            pdb.set_trace()
+        set_trace_if(debug)
 
         if extra_update_image_command is None:
             extra_update_image_command = []
@@ -568,8 +570,8 @@
         # fresh Ubuntu images and cause havoc if the locales they refer to are
         # not available. We kill them here to ease bootstrapping, then we
         # later modify the image to prevent sshd from accepting them.
-        os.environ.pop("LANG", None)
-        os.environ.pop("LC_ALL", None)
+        for variable in ['LANG', 'LC_ALL', 'LC_TIME']:
+            os.environ.pop(variable, None)
 
         credentials = EC2Credentials.load_from_file()