[recipes.py] refactor loading for operational_arguments.

recipes.py

 #!/usr/bin/env python
 # Copyright 2015 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 """Tool to interact with recipe repositories.
 
 This tool operates on the nearest ancestor directory containing an
 infra/config/recipes.cfg.
 """
@@ skipping 15 matching lines @@
 
 ROOT_DIR = os.path.dirname(os.path.abspath(__file__))
 sys.path.insert(0, ROOT_DIR)
 
 from recipe_engine import env
 from recipe_engine import arguments_pb2
 from recipe_engine import util as recipe_util
 from google.protobuf import json_format as jsonpb
 
 
-def test(config_file, package_deps, args, op_args):
+def test(config_file, package_deps, args):
   try:
     from recipe_engine import test
   except ImportError:
     logging.error(
         'Error while importing testing libraries. You may be missing the pip'
         ' package "coverage". Install it, or use the --use-bootstrap command'
         ' line argument when calling into the recipe engine, which will install'
         ' it for you.')
     raise
 
   from recipe_engine import loader
   from recipe_engine import package
 
   universe = loader.RecipeUniverse(package_deps, config_file)
   universe_view = loader.UniverseView(universe, package_deps.root_package)
 
   # Prevent flakiness caused by stale pyc files.
   package.cleanup_pyc(package_deps.root_package.recipes_dir)
 
   return test.main(
       universe_view, raw_args=args.args,
-      engine_flags=op_args.engine_flags)
+      engine_flags=args.operational_args.engine_flags)
 
 
 def lint(config_file, package_deps, args):
   from recipe_engine import lint_test
   from recipe_engine import loader
 
   universe = loader.RecipeUniverse(package_deps, config_file)
   universe_view = loader.UniverseView(universe, package_deps.root_package)
 
   lint_test.main(universe_view, args.whitelist or [])
@@ skipping 72 matching lines @@
 
     with stream_engine.make_step_stream('Failure reason') as s:
       with s.new_log_stream('reason') as l:
         l.write_split(f.human_reason)
 
     return 1
 
   return 0
 
 
-def run(config_file, package_deps, args, op_args):
+def run(config_file, package_deps, args):
   from recipe_engine import run as recipe_run
   from recipe_engine import loader
   from recipe_engine import step_runner
   from recipe_engine import stream
   from recipe_engine import stream_logdog
 
   if args.props:
     for p in args.props:
       args.properties.update(p)
 
   def get_properties_from_operational_args(op_args):
     if not op_args.properties.property:
       return None
     return _op_properties_to_dict(op_args.properties.property)
 
+  op_args = args.operational_args
   op_properties = get_properties_from_operational_args(op_args)
   if args.properties and op_properties:
     raise ValueError(
       "Got operational args properties as well as CLI properties.")
 
   properties = op_properties
   if not properties:
     properties = args.properties
 
   properties['recipe'] = args.recipe
@@ skipping 199 matching lines @@
       help='Override a project repository path with a local one.')
   parser.add_argument(
       # Use "None" as default so that we can recognize when none of the
       # bootstrap options were passed.
       '--use-bootstrap', action='store_true', default=None,
       help='Use bootstrap/bootstrap.py to create a isolated python virtualenv'
            ' with required python dependencies.')
   parser.add_argument(
       '--disable-bootstrap', action='store_false', dest='use_bootstrap',
       help='Disables bootstrap (see --use-bootstrap)')
+
+  def operational_args_type(value):
+    op_args = arguments_pb2.Arguments()
+    with open(value) as fd:

[dnj, 2017/04/26 19:52:04]

nit: Up to you, but if you care you could make this more optimal with:

with open(value) as fd:
  data = json.load(fd)
jsonpb.ParseDict(data, op_args)
return op_args

This would load JSON from stream instead of full memory buffer. But whatever.

[iannucci, 2017/04/26 20:08:35]

oh, yeah I just transplanted this code without looking at it too much, this is a
good improvement. Done.

+      data = fd.read()
+    jsonpb.Parse(data, op_args)
+    return op_args
+
   parser.add_argument(
-      '--operational-args-path', action='store',
-      type=os.path.abspath,
+      '--operational-args-path',
+      dest='operational_args',
+      type=operational_args_type,
       help='The path to an operational Arguments file. If provided, this file '
            'must contain a JSONPB-encoded Arguments protobuf message, and will '
            'be integrated into the runtime parameters.')
 
 
 def post_process_common_args(parser, args):
   if args.command == "remote":
     # TODO(iannucci): this is a hack; remote doesn't behave like ANY other
     # commands. A way to solve this will be to allow --package to take a remote
     # repo and then simply remove the remote subcommand entirely.
@@ skipping 190 matching lines @@
           'with their documentation')
   doc_p.add_argument('recipe', nargs='?',
                      help='Restrict documentation to this recipe')
   doc_p.add_argument('--kind', default='jsonpb', choices=doc_kinds,
                      help='Output this kind of documentation')
   doc_p.set_defaults(command='doc')
 
   args = parser.parse_args()
   post_process_common_args(parser, args)
 
-  # Load/parse operational arguments.
-  op_args = arguments_pb2.Arguments()
-  if args.operational_args_path is not None:
-    with open(args.operational_args_path) as fd:
-      data = fd.read()
-    jsonpb.Parse(data, op_args)
-
   # TODO(iannucci): We should always do logging.basicConfig() (probably with
   # logging.WARNING), even if no verbose is passed. However we need to be
   # careful as this could cause issues with spurious/unexpected output. I think
   # it's risky enough to do in a different CL.
 
   if args.verbose > 0:
     logging.basicConfig()
     logging.getLogger().setLevel(logging.INFO)
   if args.verbose > 1:
     logging.getLogger().setLevel(logging.DEBUG)
@@ skipping 52 matching lines @@
       return subprocess.call(
           [
             os.path.join(env_path, 'bin', python_exe),
             '-B',  # Don't compile "pyo" binaries.
             '-E',  # Don't use PYTHON* enviornment variables.
             '-s',  # Don't use user 'site.py'.
             os.path.join(ROOT_DIR, 'recipes.py'),
           ] + sys.argv[1:])
 
     # Standard recipe engine operation.
-    return _real_main(args, op_args)
+    return _real_main(args)
 
   finally:
     # If we're using a temporary deps directory, clean it up here.
     if temp_deps_dir:
       logging.info('Cleaning up temporary deps path: %s', temp_deps_dir)
 
       # Remove as much of the temporary directory as we can. If something goes
       # wrong, log the error, but don't actually raise anything.
       def on_error(_function, path, excinfo):
         logging.error('Error cleaning up temporary deps file: %s', path,
                       exc_info=excinfo)
       shutil.rmtree(temp_deps_dir, onerror=on_error)
 
 
-def _real_main(args, op_args):
-  from recipe_engine import package, package_io
+def _real_main(args):
+  from recipe_engine import package
 
   # Commands which do not require config_file, package_deps, and other objects
   # initialized later.
   if args.command == 'remote':
     return remote(args)
 
   config_file = args.package
   repo_root = package.InfraRepoConfig().from_recipes_cfg(args.package.path)
 
   try:
     # TODO(phajdan.jr): gracefully handle inconsistent deps when rolling.
     # This fails if the starting point does not have consistent dependency
     # graph. When performing an automated roll, it'd make sense to attempt
     # to automatically find a consistent state, rather than bailing out.
     # Especially that only some subcommands refer to package_deps.
     package_deps = package.PackageDeps.create(
         repo_root, config_file, allow_fetch=not args.no_fetch,
         deps_path=args.deps_path, overrides=args.project_override)
   except subprocess.CalledProcessError:
     # A git checkout failed somewhere. Return 2, which is the sign that this is
     # an infra failure, rather than a test failure.
     return 2
 
   if args.command == 'fetch':
     # We already did everything in the create() call above.
     assert not args.no_fetch, 'Fetch? No-fetch? Make up your mind!'
     return 0
   elif args.command == 'test':
-    return test(config_file, package_deps, args, op_args)
+    return test(config_file, package_deps, args)
   elif args.command == 'bundle':
     return bundle(config_file, package_deps, args)
   elif args.command == 'lint':
     return lint(config_file, package_deps, args)
   elif args.command == 'run':
-    return run(config_file, package_deps, args, op_args)
+    return run(config_file, package_deps, args)
   elif args.command == 'autoroll':
     return autoroll(repo_root, config_file, args)
   elif args.command == 'depgraph':
     return depgraph(config_file, package_deps, args)
   elif args.command == 'refs':
     return refs(config_file, package_deps, args)
   elif args.command == 'doc':
     return doc(config_file, package_deps, args)
   else:
     print """Dear sir or madam,
@@ skipping 27 matching lines @@
 
   if not isinstance(ret, int):
     if ret is None:
       ret = 0
     else:
       print >> sys.stderr, ret
       ret = 1
   sys.stdout.flush()
   sys.stderr.flush()
   os._exit(ret)