launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/gina-help into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/launchpad/gina-help/+merge/74978

= Summary =

Gina isn't very helpful; in particular, what a "target" is and where it comes from is a little obscure.


== Proposed fix ==

Add a usage string and an option to list configured targets.  Drop enough hints in code, comments, and output that it's clearer where the targets come from.


== Pre-implementation notes ==

The notion that this wasn't clear enough arose from a call with Julian.  If he doesn't know, it's not clear enough.


== Tests ==

I haven't written any new tests for this; just doesn't seem worth it.  But do run:
{{{
./bin/test -vvc lp.soyuz -t gina
}}}


== Demo and Q/A ==

Try on dogfood:
{{{
./scripts/gina.py -l
}}}

Should print out configured targets, starting with sid.


= Launchpad lint =

As usual in scripts, one piece of lint we can't fix:


Checking for conflicts and issues in changed files.

Linting changed files:
  scripts/gina.py

./scripts/gina.py
      26: '_pythonpath' imported but unused
-- 
https://code.launchpad.net/~jtv/launchpad/gina-help/+merge/74978
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/gina-help into lp:launchpad.

=== modified file 'scripts/gina.py'
--- scripts/gina.py	2011-09-08 08:00:10 +0000
+++ scripts/gina.py	2011-09-12 12:15:23 +0000
@@ -315,20 +315,44 @@
     def __init__(self):
         super(Gina, self).__init__(name='gina', dbuser=config.gina.dbuser)
 
+    @property
+    def usage(self):
+        return "%s [options] (targets|--all)" % sys.argv[0]
+
     def add_my_options(self):
+        self.parser.add_option("-a", "--all", action="store_true",
+            help="Run all sections defined in launchpad.conf (in order)",
+            dest="all", default=False)
+        self.parser.add_option("-l", "--list-targets", action="store_true",
+            help="List configured import targets", dest="list_targets",
+            default=False)
         self.parser.add_option("-n", "--dry-run", action="store_true",
             help="Don't commit changes to the database",
             dest="dry_run", default=False)
-        self.parser.add_option("-a", "--all", action="store_true",
-            help="Run all sections defined in launchpad.conf (in order)",
-            dest="all", default=False)
-
-    def main(self):
-        possible_targets = [target.category_and_section_names[1]
-            for target in config.getByCategory('gina_target')]
+
+    def getConfiguredTargets(self):
+        """Get the configured import targets.
+
+        Gina's targets are configured as "[gina_target.*]" sections in the
+        LAZR config.
+        """
+        sections = config.getByCategory('gina_target', [])
+        targets = [
+            target.category_and_section_names[1] for target in sections]
+        if len(targets) == 0:
+            self.logger.warn("No gina_target entries configured.")
+        return targets
+
+    def listTargets(self, targets):
+        """Print out the given targets list."""
+        for target in targets:
+            self.logger.info("Target: %s", target)
+
+    def getTargets(self, possible_targets):
+        """Get targets to process."""
         targets = self.args
         if self.options.all:
-            targets = possible_targets[:]
+            return list(possible_targets)
         else:
             if not targets:
                 self.parser.error(
@@ -337,8 +361,16 @@
                 if target not in possible_targets:
                     self.parser.error(
                         "No Gina target %s in config file" % target)
-
-        for target in targets:
+            return targets
+
+    def main(self):
+        possible_targets = self.getConfiguredTargets()
+
+        if self.options.list_targets:
+            self.listTargets(possible_targets)
+            return
+
+        for target in self.getTargets(possible_targets):
             target_section = config['gina_target.%s' % target]
             run_gina(self.options, self.txn, target_section)