Actually handle booleans properly in gatekeeper.

scripts/slave/gatekeeper_ng_config.py

 #!/usr/bin/env python
 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Loads gatekeeper configuration files for use with gatekeeper_ng.py.
 
 The gatekeeper json configuration file has two main sections: 'masters'
 and 'categories.' The following shows the breakdown of a possible config,
 but note that all nodes are optional (including the root 'masters' and
 'categories' nodes).
 
 A builder ultimately needs 4 lists (sets):
   closing_steps: steps which close the tree on failure or omission
   forgiving_steps: steps which close the tree but don't email committers
   tree_notify: any additional emails to notify on tree failure
   sheriff_classes: classes of sheriffs to notify on build failure
 
 Builders can inherit these properties from categories, they can inherit
 tree_notify and sheriff_classes from their master, and they can have these
 properties assigned in the builder itself. Any property not specified
 is considered blank (empty set), and inheritance is always constructive (you
 can't remove a property by inheriting or overwriting it). Builders can inherit
 categories from their master.
 
 A master consists of zero or more sections, which specify which builders are
-watched by the section and what action should be taken. A section can specify
-tree_closing to be false, which causes the section to only send out emails
-instead of closing the tree. A section or builder can also specify to respect
-a build's failure status with respect_build_status.
+watched by the section and what action should be taken.
 
 The 'excluded_builders' key is a list of builder names that will not be
 processed even if they match a configuration. This is useful when the builder
 set is specified using the wildcard ('*'). Entries in this list may use
 filename-style globbing (e.g., *mybuilder*) to specify builder name patterns.
 
 The 'subject_template' key is the template used for the email subjects. Its
 formatting arguments are found at https://chromium.googlesource.com/chromium/
   tools/chromium-build/+/master/gatekeeper_mailer.py, but the list is
 reproduced here:
@@ skipping 12 matching lines @@
 'forgive_all' converts all closing_steps to be forgiving_steps. Since
 forgiving_steps only email sheriffs + watchlist (not the committer), this is a
 great way to set up experimental or informational builders without spamming
 people. It is enabled by providing the string 'true'.
 
 'forgiving_optional' and 'closing_optional' work just like 'forgiving_steps'
 and 'closing_steps', but they won't close if the step is missing. This is like
 previous gatekeeper behavior. They can be set to '*', which will match all
 steps in the builder.
 
+'respect_build_status' means to use the buildbot result of the entire build
+as an additional way to close the tree. As an example, if a build's closing
+steps succeeded but the overall build result was FAILURE, the tree would
+close if respect_build_status is set to True. respect_build_status only checks
+for FAILURE, not any of the other statuses (including EXCEPTION). A build
+status of SUCCESS will not override failing closing or forgiving steps.
+respect_build_status is a boolean (true or false in JSON) and defaults to
+False.

[Dirk Pranke, 2016/03/16 22:29:52]

This seems kinda confusing (the concept, not the docs): why would we have
situations where all steps passed but the build failed, or a step failed but the
build passed, i.e., why is build_status != all(step['status'] == SUCCESS for
step in steps)?

[ghost stip (do not use), 2016/03/16 22:56:16]

it's the return code of the build. think of what you see on the buildbot
waterfall -- you see a single 'failed' or 'passed,' which is actually determined
by arbitrary internal buildbot logic. with recipes this becomes less relevant
over time. I don't think anyone has used it, and it would be interesting to
remove it in the future.

+
+'close_tree' allows masters or builders to disable the --set-status option
+set in gatekeeper_trees.json. In particular, this would be useful for a specific
+builder on a tree-closing master which should notify the blamelist about
+failures but should not close the tree. close_tree is a boolean (true or false
+in JSON) and defaults to True.
+
 Note that if a builder sets something as forgiving_optional which is set as
 closing_optional in the master config, this value will be removed from
 closing_optional. This allows builders to override master configuration values.
 
 The 'comment' key can be put anywhere and is ignored by the parser.
 
 # Python, not JSON.
 {
   'masters': {
     'http://build.chromium.org/p/chromium.win': [
@@ skipping 50 matching lines @@
 
 DATA_DIR = os.path.dirname(os.path.abspath(__file__))
 
 
 # Keys which have defaults besides None or set([]).
 DEFAULTS = {
     'status_template': ('Tree is closed (Automatic: "%(unsatisfied)s" on '
                         '"%(builder_name)s" %(blamelist)s)'),
     'subject_template': ('buildbot %(result)s in %(project_name)s on '
                          '%(builder_name)s, revision %(revision)s'),
+    'respect_build_status': False,
 }
 
 
 def allowed_keys(test_dict, *keys):
   keys = keys + ('comment',)
   assert all(k in keys for k in test_dict), (
       'not valid: %s; allowed: %s' % (
           ', '.join(set(test_dict.keys()) - set(keys)),
           ', '.join(keys)))
 
@@ skipping 22 matching lines @@
     if union:
       raise ValueError(
         "The builder categories have conflicting entries %s for keys %s "
         "and %s." % (union, k, v))
 
 
 def load_gatekeeper_config(filename):
   """Loads and verifies config json, constructs builder config dict."""
 
   # Keys which are allowed in a master or builder section.
-  master_keys = ['excluded_builders',
+  master_keys = ['close_tree',
+                 'excluded_builders',
                  'excluded_steps',
                  'forgive_all',
+                 'respect_build_status',
                  'sheriff_classes',
                  'status_template',
                  'subject_template',
                  'tree_notify',
   ]
 
-  builder_keys = ['closing_optional',
+  builder_keys = ['close_tree',
+                  'closing_optional',
                   'closing_steps',
                   'excluded_builders',
                   'excluded_steps',
                   'forgive_all',
                   'forgiving_optional',
                   'forgiving_steps',
+                  'respect_build_status',
                   'sheriff_classes',
                   'status_template',
                   'subject_template',
                   'tree_notify',
   ]
 
   # These keys are strings instead of sets. Strings can't be merged,
   # so more specific (master -> category -> builder) strings clobber
   # more generic ones.
   strings = ['forgive_all', 'status_template', 'subject_template']
 
+  # Bools also share the 'strings' clobbering logic.
+  bools = ['close_tree', 'respect_build_status']
+
   with open(filename) as f:
     raw_gatekeeper_config = json.load(f)
 
   allowed_keys(raw_gatekeeper_config, 'categories', 'masters')
 
   categories = raw_gatekeeper_config.get('categories', {})
   masters = raw_gatekeeper_config.get('masters', {})
 
   for category in categories.values():
     allowed_keys(category, *builder_keys)
 
   gatekeeper_config = {}
   for master_url, master_sections in masters.iteritems():
     for master_section in master_sections:
       gatekeeper_config.setdefault(master_url, []).append({})
-      allowed_keys(master_section, 'builders', 'categories', 'close_tree',
-                   'respect_build_status', *master_keys)
+      allowed_keys(master_section, 'builders', 'categories', *master_keys)
 
       builders = master_section.get('builders', {})
       for buildername, builder in builders.iteritems():
         allowed_keys(builder, 'categories', *builder_keys)
         for key, item in builder.iteritems():
           if key in strings:
             assert isinstance(item, basestring)
+          elif key in bools:
+            assert isinstance(item, bool)
           else:
             assert isinstance(item, list)
             assert all(isinstance(elem, basestring) for elem in item)
 
         gatekeeper_config[master_url][-1].setdefault(buildername, {})
         gatekeeper_builder = gatekeeper_config[master_url][-1][buildername]
 
         # Populate with specified defaults.
         for k in builder_keys:
           if k in DEFAULTS:
             gatekeeper_builder.setdefault(k, DEFAULTS[k])
           elif k in strings:
             gatekeeper_builder.setdefault(k, '')
+          elif k in bools:
+            gatekeeper_builder.setdefault(k, True)
           else:
             gatekeeper_builder.setdefault(k, set())
 
         # Inherit any values from the master.
         for k in master_keys:
-          if k in strings:
+          if k in strings or k in bools:
             if k in master_section:
               gatekeeper_builder[k] = master_section[k]
           else:
             gatekeeper_builder[k] |= set(master_section.get(k, []))
 
-        gatekeeper_builder['close_tree'] = master_section.get('close_tree',
-                                                              True)
-        gatekeeper_builder['respect_build_status'] = master_section.get(
-            'respect_build_status', False)
-
         # Inherit any values from the categories.
         for c in master_section.get('categories', []):
           for k in builder_keys:
-            if k in strings:
+            if k in strings or k in bools:
               if k in categories[c]:
                 gatekeeper_builder[k] = categories[c][k]
             else:
               gatekeeper_builder[k] |= set(categories[c].get(k, []))
 
         special_keys = {
           'forgiving': 'closing',
           'forgiving_optional': 'closing_optional',
         }
 
         check_builder_conflicts(
             special_keys, builder.get('categories', []), categories)
 
         for c in builder.get('categories', []):
           for k in builder_keys:
-            if k in strings:
+            if k in strings or k in bools:
               if k in categories[c]:
                 gatekeeper_builder[k] = categories[c][k]
             else:
               gatekeeper_builder[k] |= set(categories[c].get(k, []))
 
         # If we're forgiving something in the builder that we set as
         # closing in the master config, then don't close on it. Builders
         # can override master configurations.
         for key, key_to_modify in special_keys.items():
           if key_to_modify in gatekeeper_builder:
             gatekeeper_builder[key_to_modify] -= set(
                 gatekeeper_builder.get(key, []))
 
         # Add in any builder-specific values.
         for k in builder_keys:
-          if k in strings:
+          if k in strings or k in bools:
             if k in builder:
               gatekeeper_builder[k] = builder[k]
           else:
             gatekeeper_builder[k] |= set(builder.get(k, []))
 
         # Builder postprocessing.
         if gatekeeper_builder['forgive_all'] == 'true':
           gatekeeper_builder['forgiving_steps'] |= gatekeeper_builder[
               'closing_steps']
           gatekeeper_builder['forgiving_optional'] |= gatekeeper_builder[
@@ skipping 94 matching lines @@
     gatekeeper_config = inject_hashes(gatekeeper_config)
 
   flatten_to_json(gatekeeper_config, sys.stdout)
   print
 
   return 0
 
 
 if __name__ == '__main__':
   sys.exit(main())