Merge lp:~achiang/hydrazine/batch-operations into lp:hydrazine

[Alex Chiang]

batchify existing commands

Lots of user-visible documentation written, and even more developer documentation in the individual commits.

Please take a look and let me know what you think.

Depends on:
https://code.launchpad.net/~achiang/hydrazine/encapsulate-validation/+merge/78697

Thanks.

[Martin Packman (gz)]

This looks really interesting. So the main aim of the change is to generalise the way of showing new bugs into a new 'filter' command.

What kind of performance do you get against staging with the batch command? I don't know the launchpad api all that well and am a little concerned that just delegating out to the other do_ methods for each bug is not the best way of changing lots of bugs. Currently having an lp_save in each command isn't do bad because it's generally limited by your typing speed, but if changing importance, validity and tags on 100 bugs means 300 saves, I don't know if that's will get us yelled at.

Merge proposal nit, when you have a branch that depends on a change you've proposed separately, remember to set the 'prerequisite branch' option so those changes aren't show in the diff.

review: Needs Information

[Martin Pool (mbp)]

This would be a great feature to have, thanks achiang!

[fix] Please mention this in the readme.

[aside] It might be nice to actually split out the concept of 'active filter' into an object so that we can refine it later, but that can be changed later on.

> What kind of performance do you get against staging with the batch command?
> Currently having an lp_save in each command isn't do bad because it's generally limited by your typing speed, but if changing importance, validity and tags on 100 bugs means 300 saves, I don't know if that's will get us yelled at.

I think the thing that _will_ get us yelled at is if a slip of the finger in a batch command ends up with thousands of bugs accidentally being closed. So that's probably best done through [not having any bugs ;-)] and making it clear what's going to be done before anything happens, or at least before too much of it happens to be irretrievable. For instance we could print 'setting 213 bugs to status closed' as we start.

With performance: generally speaking having a new feature that's slow is better than not having the feature at all, as long as it's not done in a way that will be inordinately hard to improve later. And, in this case, we're automating something previously done manually so even slow operation can only help.

Each lp_save will do a blocking round trip, so if you update 100 bugs it will take some time (depending on rtt). Launchpad doesn't have a 'update multiple bugs' rpc, so to do better we either need to add new apis or (perhaps more likely) overlap multiple requests. The latter may be hard from lplib, and we might need eventually to go to twisted or golang or whatever, but either of them is out of scope.

So in short: good question, but it shouldn't block merging this imo.

[aside] This is getting to the point where it needs some testing, preferably against a mock Launchpad.

[aside] Perhaps hydrazine should print a 'what's new' when it starts.

[fix] 'filter tags' doesn't seem to be implemented?

review: Needs Fixing

[lp:~achiang/hydrazine/batch-operations]

100. By Alex Chiang on 2011-10-11

bugclient: merge with trunk to get NamedEnum() class

Get latest NamedEnum() class from trunk, and convert our old filter
enum to use it properly.

101. By Alex Chiang on 2011-10-11

README: updated to reflect new batch and filter commands

Update the readme, now that select_new is gone.

Add some helpful examples on how to use filter and batch.

[Alex Chiang (achiang)]

Hello gz,

> This looks really interesting. So the main aim of the change is to generalise the way of showing new bugs into a new 'filter' command.

Actually, processing new bugs was quite a minor consideration, it just happens that the new batch and filter commands can be used to do so (hopefully a good sign for the general soundness of the design!).

The main impetus is that I'm involved with projects where we target 30--60 bugs to a milestone, and as developers fix them, they move status to Fix Committed. After the project is QA'ed and released, the milestone is closed, but now we have to change the status on all those bugs to Fix Released.

I have future plans to extend the filters even further, so we can do things like, "select bugs marked fix committed last week", but that will obviously wait for a future MP.

Like poolie, I would prefer to shelve the performance discussion for a later MP as well.

I've updated README as requested by poolie.

Would love it if folks could re-review, thanks!

[Martin Pool (mbp)]

thanks for the doc updates.

72 +Caution, without the 'filter' command, you will operate on ALL
73 +active bugs in your pillar, including bugs already Fix Committed.

[fix] That sounds kind of risky. Would it be at all practical to have it just fail if there is no filter? I think it would, just by after the 'for f' block

   else:
        print 'No filter set?'
        return

aside from that it looks good - thanks!

If you just fix that thing, please go ahead and land it.

review: Needs Fixing

[lp:~achiang/hydrazine/batch-operations]

102. By Alex Chiang on 2011-10-12

bugclient: require a filter before doing batch commands

This requirement makes life a little safer. Without it, we had the
potential to do a batch operation on ALL active bugs in the pillar,
which includes bugs with status 'Fix Committed'.

If we require a filter, that can mitigate this little pitfall.

[Alex Chiang (achiang)]

Fixed, and now landing, per poolie's approval.

Thanks all!

=== modified file 'README'
--- README	2010-09-02 01:16:39 +0000
+++ README	2011-10-12 03:20:22 +0000
@@ -20,13 +20,35 @@
 
   ./bugclient 
   pillar bzr 
-  select_new  
+  filter status new
+  batch
   bug 1234
   triage confirmed high +dirstate
   next
 
 ``triage`` can set the importance, status, and tags in one go.
 
+In the example above, we use the plain ``batch`` command after selecting all
+bugs with status New, and iterate over them one at a time, with ``next``.
+
+A more interesting example of how to use the batch command might be to
+change the status of many bugs after a project has released a milestone::
+
+   ./bugclient
+   pillar hydrazine
+   filter milestone 0.2
+   filter status Fix Committed		# note, you may apply multiple filters
+   batch status Fix Released
+
+Or perhaps you would like to tag multiple bugs at once::
+
+   ./bugclient
+   pillar hydrazine
+   filter importance wishlist
+   batch tag +bluesky
+
+See the help for ``filter`` and ``batch`` for more details.
+
 feed-pqm
 ========
 

=== modified file 'bugclient'
--- bugclient	2011-10-11 02:39:41 +0000
+++ bugclient	2011-10-12 03:20:22 +0000
@@ -38,6 +38,7 @@
                            'Incomplete', 'Invalid', 'New'])
 importance_enum = NamedEnum(['Critical', 'High', 'Medium', 'Low',
                               'Wishlist', 'Undecided'])
+filter_enum = NamedEnum(['milestone', 'status', 'tags', 'clear'])
 
 class HydrazineCmd(cmd.Cmd):
 
@@ -47,10 +48,61 @@
         self.current_bug_number = None
         self.pillar = None
         self.task_list = None
+        self.filters = {}
 
     def _connect(self):
         self.session = hydrazine.create_session()
 
+    def do_batch(self, line):
+        """Perform batch operation on current pillar's bugs.
+Use the 'filter' command to limit the batch command's scope of bugs.
+
+example:
+    filter status new
+    batch                           # load New bugs; iterate manually with next
+    batch show                      # show information on all New bugs
+    batch status confirmed          # change all New bugs to Confirmed
+
+Caution, there is very little error checking. You are about to commit
+a lot of changes to a lot of bugs. Double-check your command!
+        """
+        kwargs = {}
+        if not self.filters:
+            print 'No filter set?'
+            return
+        for f in self.filters:
+            if f == "milestone":
+                kwargs[f] = "%s/+milestone/%s" % (self.pillar, self.filters[f])
+            else:
+                kwargs[f] = self.filters[f]
+        # XXX: is this the best sort order?
+        kwargs['order_by'] = '-datecreated'
+        self.task_list = self.pillar.searchTasks(**kwargs)
+
+        try:
+            first_bug_task = self.task_list[0]
+            self.search_index = 0
+            self._select_bug(first_bug_task.bug, "quiet")
+        except IndexError:
+            print "No bugtasks found"
+            return
+
+        args = line.split()
+        if not args:
+            print "Task list loaded"
+            self._show_bug(self.bug)
+            return
+
+        # XXX: blind pass-through for now; need to prevent commands that
+        # make no sense in batch mode, such as 'official_tags', etc.
+        for tasks in self.task_list:
+            getattr(self, "do_" + args[0])(' '.join(args[1:]))
+            self.do_next("quiet")
+
+        # go back to beginning of task_list to do more batch commands
+        self.search_index = 0
+        self._select_bug(self.task_list[0].bug, "quiet")
+
     def do_bug(self, bug_number):
         """Open bug by number"""
         try:
@@ -106,6 +158,36 @@
     def do_EOF(self, what):
         return True
 
+    def do_filter(self, line):
+        """Add a filter for batch operations
+
+examples:
+    filter milestone M1                  # milestone named 'M1'
+    filter status new                    # all New bugs
+    filter tags a b c                    # tags 'a', 'b', and 'c'
+    filter clear                         # remove all filters
+        """
+        args = line.split()
+        if not filter_enum.get(args[0]):
+            print 'Invalid filter. Valid filters are:'
+            _show_columnated(filter_enum.get_all())
+            return
+
+        if args[0] == "clear":
+            self.filters = {}
+            return
+        elif args[0] == "status":
+            if status_enum.get(' '.join(args[1:])) == None:
+                print 'invalid status, valid values are:'
+                _show_columnated(status_enum.get_all())
+                return
+            else:
+                # Normalize status, and toss old junk
+                args[1] = status_enum.get(' '.join(args[1:]))
+                del args[2:]
+
+        self.filters[args[0]] = ' '.join(args[1:])
+
     def do_importance(self, line):
         """Set importance"""
         task = self._needs_single_task()
@@ -116,10 +198,10 @@
             print 'invalid importance, valid values are:'
             _show_columnated(importance_enum.get_all())
             return
-        print 'changing importance %s => %s' % (task.importance, new_importance)
+        print '#%d changing importance %s => %s' % (self.bug.id, task.importance, new_importance)
         task.importance = new_importance
-        print '**** before save'
         if opts.debug:
+            print '**** before save'
             print task._wadl_resource._definition.representation
         try:
             task.lp_save()
@@ -128,10 +210,10 @@
             if opts.debug:
                 print task._wadl_resource._definition.representation
 
-    def do_next(self, ignored):
+    def do_next(self, show):
         """Go to the next bug in the list"""
         if self.task_list is None:
-            print 'no list loaded; use select_new etc'
+            print 'no list loaded; use a filter and batch to load a list'
             return
         self.search_index += 1
         try:
@@ -139,7 +221,9 @@
         except IndexError:
             print "End of bug list"
             return
-        self._select_bug(bug_task.bug)
+        if show == "":
+            show = "show"
+        self._select_bug(bug_task.bug, show)
 
     def do_official_tags(self, ignored):
         """Show the official tags for the current pillar."""
@@ -181,19 +265,6 @@
         task.target = new_target
         task.lp_save()
 
-    def do_select_new(self, ignored):
-        """Select the list of new bugs in the current pillar"""
-        if self._needs_pillar(): return
-        self.task_list = self.pillar.searchTasks(status="New",
-            order_by=['-datecreated'])
-        self.task_list_index = 0
-        try:
-            first_bug_task = self.task_list[0]
-            self.search_index = 0
-        except IndexError:
-            print "No bugtasks found"
-        self._select_bug(first_bug_task.bug)
-
     def do_show(self, ignored):
         """Show the header of the current bug"""
         if self._needs_bug():
@@ -229,7 +300,7 @@
             print 'invalid status, valid values are:'
             _show_columnated(status_enum.get_all())
             return
-        print 'changing status %s => %s' % (task.status, new_status)
+        print '#%d changing status %s => %s' % (self.bug.id, task.status, new_status)
         task.status = new_status
         task.lp_save()
 
@@ -343,7 +414,10 @@
 
     @property 
     def prompt(self):
-        p = 'hydrazine(%s) ' % (self.short_service_root,)
+        p = ""
+        if self.filters:
+            p = 'Current filters: %s\n' % self.filters
+        p += 'hydrazine(%s) ' % (self.short_service_root,)
         if self.current_bug_number is not None:
             p += '#%d ' % (self.current_bug_number,)
         if self.pillar is not None:
@@ -355,10 +429,11 @@
             p = p[:-1]
         return p + '> '
 
-    def _select_bug(self, the_bug):
+    def _select_bug(self, the_bug, show="show"):
         self.bug = the_bug
         self.current_bug_number = the_bug.id
-        self._show_bug(self.bug)
+        if show == "show":
+            self._show_bug(self.bug)
 
     def _find_pillar(self, pillar_name):
         pillar_collection = self.session.pillars.search(text=pillar_name)