Merge pull request #1211 from minrk/cli-defaults

reconcile CLI/config priority

Author: consideRatio

repo2docker/__main__.py

@@ -93,7 +93,7 @@ def get_argparser():
 
     argparser.add_argument(
         "--json-logs",
-        default=False,
+        default=None,
         action="store_true",
         help="Emit JSON logs instead of human readable logs",
     )
@@ -108,12 +108,13 @@ def get_argparser():
 
     argparser.add_argument(
         "--image-name",
-        help=("Name of image to be built. If unspecified will be " "autogenerated"),
+        help="Name of image to be built. If unspecified will be autogenerated",
         type=validate_image_name,
     )
 
     argparser.add_argument(
         "--ref",
+        default=None,
         help=(
             "Reference to build instead of default reference. For example"
             " branch name or commit for a Git repository."
@@ -126,10 +127,16 @@ def get_argparser():
         "--no-build",
         dest="build",
         action="store_false",
-        help=(
-            "Do not actually build the image. Useful in conjunction " "with --debug."
-        ),
+        help="Do not actually build the image. Useful in conjunction with --debug.",
+    )
+
+    argparser.add_argument(
+        "--build",
+        dest="build",
+        action="store_true",
+        help="Build the image (default)",
     )
+    argparser.set_defaults(build=None)
 
     argparser.add_argument(
         "--build-memory-limit",
@@ -149,6 +156,14 @@ def get_argparser():
         help="Do not run container after it has been built",
     )
 
+    argparser.add_argument(
+        "--run",
+        dest="run",
+        action="store_true",
+        help="Run container after it has been built (default).",
+    )
+    argparser.set_defaults(run=None)
+
     argparser.add_argument(
         "--publish",
         "-p",
@@ -164,6 +179,7 @@ def get_argparser():
         "--publish-all",
         "-P",
         dest="all_ports",
+        default=None,
         action="store_true",
         help="Publish all exposed ports to random host ports.",
     )
@@ -174,13 +190,27 @@ def get_argparser():
         action="store_false",
         help="Don't clean up remote checkouts after we are done",
     )
+    argparser.add_argument(
+        "--clean",
+        dest="clean",
+        action="store_true",
+        help="Clean up remote checkouts after we are done (default).",
+    )
+    argparser.set_defaults(clean=None)
 
     argparser.add_argument(
         "--push",
         dest="push",
         action="store_true",
         help="Push docker image to repository",
     )
+    argparser.add_argument(
+        "--no-push",
+        dest="push",
+        action="store_false",
+        help="Don't push docker image to repository (default).",
+    )
+    argparser.set_defaults(push=None)
 
     argparser.add_argument(
         "--volume",
@@ -250,6 +280,7 @@ def get_argparser():
     return argparser
 
 
+# Note: only used by sphinx-autoprogram
 argparser = get_argparser()
 
 
@@ -274,12 +305,18 @@ def make_r2d(argv=None):
     args, traitlet_args = argparser.parse_known_args(argv)
 
     r2d = Repo2Docker()
-    r2d.parse_command_line(traitlet_args)
 
     if args.debug:
         r2d.log_level = logging.DEBUG
 
+    # load CLI after config file, for correct priority
     r2d.load_config_file(args.config)
+    r2d.parse_command_line(traitlet_args)
+
+    if args.debug:
+        # re-apply debug in case log_level was also set via config
+        r2d.log_level = logging.DEBUG
+
     if args.appendix:
         r2d.appendix = args.appendix
 
@@ -291,8 +328,14 @@ def make_r2d(argv=None):
         key, _, val = a.partition("=")
         r2d.extra_build_args[key] = val
 
+    # repo is a required arg, and should never come from config:
+    if "repo" in r2d.config.Repo2Docker:
+        r2d.log.warning(
+            f"Ignoring Repo2Docker.repo={r2d.repo!r} configuration, using {args.repo!r} from CLI."
+        )
     r2d.repo = args.repo
-    r2d.ref = args.ref
+    if args.ref is not None:
+        r2d.ref = args.ref
 
     # user wants to mount a local directory into the container for
     # editing
@@ -313,21 +356,22 @@ def make_r2d(argv=None):
 
     if args.image_name:
         r2d.output_image_spec = args.image_name
-    else:
-        # we will pick a name after fetching the repository
-        r2d.output_image_spec = ""
 
-    r2d.json_logs = args.json_logs
+    if args.json_logs is not None:
+        r2d.json_logs = args.json_logs
 
-    r2d.dry_run = not args.build
+    if args.build is not None:
+        r2d.dry_run = not args.build
 
     if r2d.dry_run:
         # Can't push nor run if we aren't building
         args.run = False
         args.push = False
 
-    r2d.run = args.run
-    r2d.push = args.push
+    if args.run is not None:
+        r2d.run = args.run
+    if args.push is not None:
+        r2d.push = args.push
 
     # check against r2d.run and not args.run as r2d.run is false on
     # --no-build. Also r2d.volumes and not args.volumes since --editable
@@ -341,29 +385,33 @@ def make_r2d(argv=None):
         src, dest = v.split(":")
         r2d.volumes[src] = dest
 
-    r2d.run_cmd = args.cmd
+    if args.cmd:
+        r2d.run_cmd = args.cmd
 
     if args.all_ports and not r2d.run:
-        print(
-            "To publish user defined port mappings, the container must " "also be run"
-        )
+        print("To publish user-defined port mappings, the container must also be run")
         sys.exit(1)
 
     if args.ports and not r2d.run:
-        print(
-            "To publish user defined port mappings, the container must " "also be run"
-        )
+        print("To publish user-defined port mappings, the container must also be run")
         sys.exit(1)
 
     if args.ports and len(args.ports) > 1 and not r2d.run_cmd:
         print(
-            "To publish user defined port mapping, user must specify "
-            "the command to run in the container"
+            "To publish user-defined port mapping, "
+            "you must specify the command to run in the container"
         )
         sys.exit(1)
 
-    r2d.ports = validate_and_generate_port_mapping(args.ports)
-    r2d.all_ports = args.all_ports
+    if args.ports:
+        # override or update, if also defined in config file?
+        if r2d.ports:
+            r2d.log.warning(
+                f"Ignoring port configuration from config (ports={r2d.ports}), overridden by CLI"
+            )
+        r2d.ports = validate_and_generate_port_mapping(args.ports)
+    if args.all_ports is not None:
+        r2d.all_ports = args.all_ports
 
     if args.user_id:
         r2d.user_id = args.user_id
@@ -401,13 +449,11 @@ def make_r2d(argv=None):
     if args.engine:
         r2d.engine = args.engine
 
-    r2d.environment = args.environment
+    if args.environment:
+        # extend any environment config from a config file
+        r2d.environment.extend(args.environment)
 
-    # if the source exists locally we don't want to delete it at the end
-    # FIXME: Find a better way to figure out if repo is 'local'. Push this into ContentProvider?
-    if os.path.exists(args.repo):
-        r2d.cleanup_checkout = False
-    else:
+    elif args.clean is not None:
         r2d.cleanup_checkout = args.clean
 
     if args.target_repo_dir:

repo2docker/app.py

@@ -20,7 +20,7 @@
 import entrypoints
 import escapism
 from pythonjsonlogger import jsonlogger
-from traitlets import Any, Bool, Dict, Int, List, Unicode, default
+from traitlets import Any, Bool, Dict, Int, List, Unicode, default, observe
 from traitlets.config import Application
 
 from . import __version__, contentproviders
@@ -304,7 +304,7 @@ def _user_name_default(self):
     )
 
     cleanup_checkout = Bool(
-        False,
+        True,
         help="""
         Delete source repository after building is done.
 
@@ -313,6 +313,12 @@ def _user_name_default(self):
         config=True,
     )
 
+    @default("cleanup_checkout")
+    def _defaut_cleanup_checkout(self):
+        # if the source exists locally we don't want to delete it at the end
+        # FIXME: Find a better way to figure out if repo is 'local'. Push this into ContentProvider?
+        return not os.path.exists(self.repo)
+
     output_image_spec = Unicode(
         "",
         help="""
@@ -332,7 +338,7 @@ def _user_name_default(self):
     )
 
     run = Bool(
-        False,
+        True,
         help="""
         Run docker image after building
         """,
@@ -349,6 +355,12 @@ def _user_name_default(self):
         config=True,
     )
 
+    @observe("dry_run")
+    def _dry_run_changed(self, change):
+        if change.new:
+            # dry_run forces run and push to be False
+            self.push = self.run = False
+
     # FIXME: Refactor classes to separate build & run steps
     run_cmd = List(
         [],
@@ -727,6 +739,8 @@ def build(self):
         # making a copy of it as it might contain large files that would be
         # expensive to copy.
         if os.path.isdir(self.repo):
+            # never cleanup when we are working in a local repo
+            self.cleanup_checkout = False
             checkout_path = self.repo
         else:
             if self.git_workdir is None:
@@ -828,6 +842,7 @@ def build(self):
 
         finally:
             # Cleanup checkout if necessary
+            # never cleanup when checking out a local repo
             if self.cleanup_checkout:
                 shutil.rmtree(checkout_path, ignore_errors=True)
 

tests/unit/test_args.py

@@ -95,3 +95,36 @@ def test_invalid_image_name():
     """
     with pytest.raises(SystemExit):
         make_r2d(["--image-name", "_invalid", "."])
+
+
+def test_build_config_priority(tmp_path):
+    config_file = str(tmp_path.joinpath("repo2docker_config.py"))
+    with open(config_file, "w") as f:
+        f.write("c.Repo2Docker.dry_run = True")
+    r2d = make_r2d(["--config", config_file, "."])
+    assert r2d.dry_run
+    r2d = make_r2d(["--config", config_file, "--build", "."])
+    assert not r2d.dry_run
+    with open(config_file, "w") as f:
+        f.write("c.Repo2Docker.dry_run = False")
+    r2d = make_r2d(["--config", config_file, "--no-build", "."])
+    assert r2d.dry_run
+
+
+@pytest.mark.parametrize(
+    "trait, arg, default",
+    [
+        ("output_image_spec", "--image-name", ""),
+        ("ref", "--ref", None),
+    ],
+)
+def test_config_priority(tmp_path, trait, arg, default):
+    config_file = str(tmp_path.joinpath("repo2docker_config.py"))
+    r2d = make_r2d(["."])
+    assert getattr(r2d, trait) == default
+    with open(config_file, "w") as f:
+        f.write(f"c.Repo2Docker.{trait} = 'config'")
+    r2d = make_r2d(["--config", config_file, "."])
+    assert getattr(r2d, trait) == "config"
+    r2d = make_r2d(["--config", config_file, arg, "cli", "."])
+    assert getattr(r2d, trait) == "cli"

tests/unit/test_argumentvalidation.py

@@ -162,7 +162,7 @@ def test_port_mapping_no_run_fail(temp_cwd):
     assert not validate_arguments(
         builddir,
         args_list,
-        "To publish user defined port mappings, the container must also be run",
+        "To publish user-defined port mappings, the container must also be run",
     )
 
 
@@ -175,7 +175,7 @@ def test_all_ports_mapping_no_run_fail(temp_cwd):
     assert not validate_arguments(
         builddir,
         args_list,
-        "To publish user defined port mappings, the container must also be run",
+        "To publish user-defined port mappings, the container must also be run",
     )