swarmbucket: refactor builder defaults

appengine/cr-buildbucket/swarming/swarmingcfg.py

 # 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.
 
 import json
 import re
 
 from proto import project_config_pb2
 
 
 DIMENSION_KEY_RGX = re.compile(r'^[a-zA-Z\_\-]+$')
+NO_PROPERTY = object()
+
+
+def copy_msg(msg):
+  """Clones a protobuf message."""

[Vadim Sh., 2016/08/03 20:58:11]

how does it handle fields that are protos themselves (i.e. is it shallow or deep
clone)? Add a note in the comment.

[nodir, 2016/08/03 21:38:11]

removed in favor of copy.deepcopy because this thing does the same
see also 
https://developers.google.com/protocol-buffers/docs/reference/python/google.p...

+  copy = type(msg)()
+  copy.CopyFrom(msg)
+  return copy
+
+
+def read_properties(recipe):
+  """Parses build properties from the recipe message.
+
+  Expects the message to be valid.
+
+  Uses NO_PROPERTY for empty values.
+  """
+  result = dict(p.split(':', 1) for p in recipe.properties)
+  for p in recipe.properties_j:
+    k, v = p.split(':', 1)
+    if not v:

[Vadim Sh., 2016/08/03 20:58:12]

I find it a bit sad that 'properties: "a:"' and 'properties_j: "a:"' mean very
different things :(

Can 'properties' be eliminated completely in favor of 'properties_j'?

[nodir, 2016/08/03 21:38:11]

unfortunately no, because

properties_j: "a:\"b\""

is worse :(

+      parsed = NO_PROPERTY
+    else:
+      parsed = json.loads(v)
+    result[k] = parsed
+  return result
+
+
+def merge_recipe(r1, r2):
+  """Merges Recipe message r2 into r1.
+
+  Expects messages to be valid.
+
+  All properties are converted to properties_j.
+  """
+  props = read_properties(r1)
+  props.update(read_properties(r2))
+
+  r1.MergeFrom(r2)
+  r1.properties[:] = []
+  r1.properties_j[:] = [
+    '%s:%s' % (k, json.dumps(v))
+    for k, v in sorted(props.iteritems())
+    if v is not NO_PROPERTY
+  ]
+
+
+def merge_dimensions(d1, d2):
+  """Merges dimensions. Values in d2 overwrite values in d1.
+
+  Expects dimensions to be valid.
+
+  If a dimensions value in d2 is empty, it is excluded from the result.
+  """
+  parse = lambda d: dict(a.split(':', 1) for a in d)
+  dims = parse(d1)
+  dims.update(parse(d2))
+  return ['%s:%s' % (k, v) for k, v in sorted(dims.iteritems()) if v]
+
+
+def merge_builder(b1, b2):
+  """Merges Builder message b2 into b1. Expects messages to be valid."""
+  dims = merge_dimensions(b1.dimensions, b2.dimensions)
+  recipe = None
+  if b1.HasField('recipe') or b2.HasField('recipe'):  # pragma: no branch
+    recipe = copy_msg(b1.recipe)
+    merge_recipe(recipe, b2.recipe)
+
+  b1.MergeFrom(b2)
+  b1.dimensions[:] = dims
+  if recipe:  # pragma: no branch
+    b1.recipe.CopyFrom(recipe)
+
+
+# TODO(nodir): remove this function once all confgis are converted
+def normalize_swarming_cfg(cfg):
+  """Converts deprecated fields into new ones.
+
+  Does not check for presence of both new and deprecated fields, because
+  configs will be migrated shortly after this change is deployed.
+  """
+  if cfg.HasField('builder_defaults'):  # pragma: no cover
+    return
+  defs = cfg.builder_defaults
+  defs.swarming_tags[:] = cfg.common_swarming_tags
+  defs.dimensions[:] = cfg.common_dimensions
+  defs.recipe.CopyFrom(cfg.common_recipe)
+  defs.execution_timeout_secs = cfg.common_execution_timeout_secs
 
 
 def validate_tag(tag, ctx):
   # a valid swarming tag is a string that contains ":"
   if ':' not in tag:
     ctx.error('does not have ":": %s', tag)
   name = tag.split(':', 1)[0]
   if name.lower() == 'builder':
     ctx.error(
         'do not specify builder tag; '
@@ skipping 15 matching lines @@
         if not DIMENSION_KEY_RGX.match(key):
           ctx.error(
             'key "%s" does not match pattern "%s"',
             key, DIMENSION_KEY_RGX.pattern)
         if key in known_keys:
           ctx.error('duplicate key %s', key)
         else:
           known_keys.add(key)
 
 
-def validate_recipe_cfg(recipe, common_recipe, ctx):
-  common_recipe = common_recipe or project_config_pb2.Swarming.Recipe()
-  if not (recipe.name or common_recipe.name):
+def validate_recipe_cfg(recipe, ctx, final=True):
+  """Validates a Recipe message.
+
+  If final is False, does not validate for completeness.
+  """
+  if final and not recipe.name:
     ctx.error('name unspecified')
-  if not (recipe.repository or common_recipe.repository):
+  if final and not recipe.repository:
     ctx.error('repository unspecified')
   validate_recipe_properties(recipe.properties, recipe.properties_j, ctx)
 
 
 def validate_recipe_properties(properties, properties_j, ctx):
   keys = set()
 
   def validate_key(key):
     if not key:
       ctx.error('key not specified')
@@ skipping 14 matching lines @@
         keys.add(key)
 
   for i, p in enumerate(properties_j):
     with ctx.prefix('properties_j #%d: ', i + 1):
       if ':' not in p:
         ctx.error('does not have colon')
       else:
         key, value = p.split(':', 1)
         validate_key(key)
         keys.add(key)
-        try:
-          json.loads(value)
-        except ValueError as ex:
-          ctx.error(ex)
+        if value:
+          try:
+            json.loads(value)
+          except ValueError as ex:
+            ctx.error(ex)
 
 
-def validate_builder_cfg(
-    builder, ctx, bucket_has_pool_dim=False, common_recipe=None):
-  if not builder.name:
+def validate_builder_cfg(builder, ctx, final=True):
+  """Validates a Builder message.
+
+  If final is False, does not validate for completeness.
+  """
+  if final and not builder.name:
     ctx.error('name unspecified')
 
   for i, t in enumerate(builder.swarming_tags):
     with ctx.prefix('tag #%d: ', i + 1):
       validate_tag(t, ctx)
 
   validate_dimensions('dimension', builder.dimensions, ctx)
-  if not bucket_has_pool_dim and not has_pool_dimension(builder.dimensions):
-    ctx.error(
-      'has no "pool" dimension. '
-      'Either define it in the builder or in "common_dimensions"')
+  if final and not has_pool_dimension(builder.dimensions):
+    ctx.error('has no "pool" dimension')
 
-  if not builder.HasField('recipe') and not common_recipe:
-    ctx.error('recipe unspecified')
-  else:
-    with ctx.prefix('recipe: '):
-      validate_recipe_cfg(builder.recipe, common_recipe, ctx)
+  with ctx.prefix('recipe: '):
+    validate_recipe_cfg(builder.recipe, ctx, final=final)
 
   if builder.priority > 200:
     ctx.error('priority must be in [0, 200] range; got %d', builder.priority)
 
 
 def validate_cfg(swarming, ctx):
+  swarming = copy_msg(swarming)
+  normalize_swarming_cfg(swarming)
+
   if not swarming.hostname:
     ctx.error('hostname unspecified')
-  for i, t in enumerate(swarming.common_swarming_tags):
-    with ctx.prefix('common tag #%d: ', i + 1):
-      validate_tag(t, ctx)
-
-  validate_dimensions('common dimension', swarming.common_dimensions, ctx)
-  has_pool_dim = has_pool_dimension(swarming.common_dimensions)
-
   if swarming.task_template_canary_percentage > 100:
     ctx.error('task_template_canary_percentage must must be in [0, 100]')
 
-  common_recipe = None
-  if swarming.HasField('common_recipe'):
-    common_recipe = swarming.common_recipe
-    with ctx.prefix('common_recipe: '):
-      validate_recipe_properties(
-          swarming.common_recipe.properties,
-          swarming.common_recipe.properties_j, ctx)
+  with ctx.prefix('builder_defaults: '):
+    if swarming.builder_defaults.name:
+      ctx.error('do not specify default name')
+    subctx = ctx.proxy(ctx)

[Vadim Sh., 2016/08/03 20:58:11]

huh? what is 'proxy'?

[nodir, 2016/08/03 21:38:11]

ugh, I forgot that validation context is in a different repo. Anyway, moved that
function here

+    validate_builder_cfg(swarming.builder_defaults, subctx, final=False)
+    builder_defaults_has_errors = subctx.result().has_errors
 
   for i, b in enumerate(swarming.builders):
     with ctx.prefix('builder %s: ' % (b.name or '#%s' % (i + 1))):
-      validate_builder_cfg(
-          b, ctx, bucket_has_pool_dim=has_pool_dim,
-          common_recipe=common_recipe)
+      # Validate b before merging, otherwise merging will fail.
+      subctx = ctx.proxy(ctx)
+      validate_builder_cfg(b, subctx, final=False)
+      if subctx.result().has_errors or builder_defaults_has_errors:
+        # Do no try to merge invalid configs.
+        continue
+
+      merged = copy_msg(swarming.builder_defaults)
+      merge_builder(merged, b)
+      validate_builder_cfg(merged, ctx)
 
 
 def has_pool_dimension(dimensions):
   return any(d.startswith('pool:') for d in dimensions)