swarmbucket: refactor builder defaults

appengine/cr-buildbucket/swarming/swarmingcfg.py

Index: appengine/cr-buildbucket/swarming/swarmingcfg.py
diff --git a/appengine/cr-buildbucket/swarming/swarmingcfg.py b/appengine/cr-buildbucket/swarming/swarmingcfg.py
index 324151cea03a0e6dbb9b5551453daf6ff99283cd..3410e12e292434fe3c155792e434b332967b4279 100644
--- a/appengine/cr-buildbucket/swarming/swarmingcfg.py
+++ b/appengine/cr-buildbucket/swarming/swarmingcfg.py
@@ -9,6 +9,94 @@ 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):
@@ -44,11 +132,14 @@ def validate_dimensions(field_name, dimensions, ctx):
           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)
 
@@ -83,15 +174,19 @@ def validate_recipe_properties(properties, properties_j, ctx):
         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, final=True):
+  """Validates a Builder message.
 
-def validate_builder_cfg(
-    builder, ctx, bucket_has_pool_dim=False, common_recipe=None):
-  if not builder.name:
+  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):
@@ -99,47 +194,44 @@ def validate_builder_cfg(
       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):