kitchen_run: initial CL

scripts/master/factory/kitchen_factory.py

Index: scripts/master/factory/kitchen_factory.py
diff --git a/scripts/master/factory/kitchen_factory.py b/scripts/master/factory/kitchen_factory.py
new file mode 100644
index 0000000000000000000000000000000000000000..a7755f7379615c0be5dbc338fb376bff42d6c7c5
--- /dev/null
+++ b/scripts/master/factory/kitchen_factory.py
@@ -0,0 +1,52 @@
+# Copyright 2016 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 copy
+
+from master.factory import annotator_commands
+from master.factory import commands
+from master.factory.build_factory import BuildFactory
+
+
+class KitchenFactory(object):

[nodir, 2016/04/12 01:04:55]

please add a class docstring

+  def __init__(self, active_master, repository, revision='origin/master',
+               common_properties=None):
+    self.active_master = active_master
+    self.repository = repository
+    self.revision = revision
+
+    self._common_properties = common_properties or {}

[iannucci, 2016/04/11 18:07:28]

hm... What do we need this for? This means we have 3 layers of properties:
common_properties, factory_properties and build_properties. Could we just get by
with factory_properties and a helper function like we do for other Factories?

In fact, could we not have KitchenFactory be a class and just have it be a
method (e.g. the body of BaseFactory)? So instead of

  F = KitchenFactory(some, options)
  buildFactory = F.BaseFactory(other options)

It's just

  buildFactory = KitchenFactory(all, of, the, options)

?


And if there are common options, just have a helper in the master

  def defaultKitchenFactory(some, of, the, options):
    return KitchenFactory(default, repo, with, some, of, the, options)

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

FWIW, this design mirrors AnnotatorFactory, and I see some value in consistency.

A use case I had in mind for common_properties was e.g. setting path_config to
kitchen. This can be done with the helper as well.

I'm fine with changes and can do that in the next patchset. Please let me know
if anybody wouldn't like it, so we avoid back-and-forth.

[nodir, 2016/04/12 01:04:54]

+1 to what Robbie said. This class has only one method.

+
+  def BaseFactory(self, recipe, factory_properties=None, timeout=1200, max_time=2400):
+    """

[nodir, 2016/04/12 01:04:55]

first line should be one sentence, then blank line, then the rest
https://google.github.io/styleguide/pyguide.html#Comments

+    |recipe| is the name of the recipe to pass to kitchen_run.
+
+    |timeout| refers to the maximum number of seconds a build should be allowed
+    to run without output. After no output for |timeout| seconds, the build is
+    forcibly killed.
+
+    |max_time| refers to the maximum number of seconds a build should be allowed
+    to run, regardless of output. After |max_time| seconds, the build is
+    forcibly killed.
+    """
+    properties = copy.copy(self._common_properties)
+    properties.update(factory_properties or {})
+    #properties.update({'recipe': recipe})

[dnj, 2016/04/11 18:17:15]

Maybe delete this parameter, since we're not using it?

[Paweł Hajdan Jr., 2016/04/11 20:44:39]

This line will go away in the final version of the CL.

Note that we do use recipe parameter below, when invoking kitchen_run.py .

+    factory = BuildFactory(build_inherit_factory_properties=False)
+    factory.properties.update(properties, 'KitchenFactory')
+    cmd_obj = annotator_commands.AnnotatorCommands(
+        factory, active_master=self.active_master)
+
+    runner = cmd_obj.PathJoin(cmd_obj.script_dir, 'kitchen_run.py')
+    cmd = [
+        cmd_obj.python, '-u', runner,
+        '--repository', self.repository,
+        '--revision', self.revision,
+        '--recipe', recipe,
+    ]
+    cmd = cmd_obj.AddB64GzBuildProperties(cmd)
+
+    cmd_obj.AddAnnotatedScript(cmd, timeout=timeout, max_time=max_time)
+
+    return factory