Repository: jazzband/pip-tools
Pull request #913 (2019-09-27 10:14:35)

Add --ask option to pip-sync
This adds an `--ask` option to pip-sync which will first show the user what would change, like `--dry-run`, and then ask to confirm before continuing with the changes. This is supposed to address issue #671 without breaking existing workflows (in particular CI pipelines) and in a way that is well-established by existing tools like nmcli (network manager) and portage (Gentoo package manager).

This change requires a minor version bump.

**Changelog-friendly one-liner**: Add `--ask` option to `pip-sync`.

##### Contributor checklist

- [x] Provided the tests for the changes.
- [x] Requested a review from another contributor.
- [x] Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
- [x] Assign the PR to an existing or new milestone for the target version (following [Semantic Versioning](https://blog.versioneye.com/2014/01/16/semantic-versioning/)).


georgek: @atugushev Please could you review?
codecov[bot]: # [Codecov](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=h1) Report
> Merging [#913](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=desc) into [master](https://codecov.io/gh/jazzband/pip-tools/commit/51463fcddb75f1d96569f1d0585970eba67949d7?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/jazzband/pip-tools/pull/913/graphs/tree.svg?width=650&token=CELIrP9w8G&height=150&src=pr)](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #913      +/-   ##
==========================================
+ Coverage   98.57%   98.59%   +0.02%     
==========================================
  Files          35       35              
  Lines        2247     2279      +32     
  Branches      288      292       +4     
==========================================
+ Hits         2215     2247      +32     
  Misses         21       21              
  Partials       11       11
```


| [Impacted Files](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [piptools/scripts/sync.py](https://codecov.io/gh/jazzband/pip-tools/pull/913/diff?src=pr&el=tree#diff-cGlwdG9vbHMvc2NyaXB0cy9zeW5jLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [tests/test\_sync.py](https://codecov.io/gh/jazzband/pip-tools/pull/913/diff?src=pr&el=tree#diff-dGVzdHMvdGVzdF9zeW5jLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [piptools/sync.py](https://codecov.io/gh/jazzband/pip-tools/pull/913/diff?src=pr&el=tree#diff-cGlwdG9vbHMvc3luYy5weQ==) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=footer). Last update [51463fc...90b8a3b](https://codecov.io/gh/jazzband/pip-tools/pull/913?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

georgek: Thanks @atugushev!

Please let me know when I should squash/rebase etc.
atugushev: @georgek 

> Please let me know when I should squash/rebase etc.

Yes, please, squash and rebase it.

Patch:
diff --git a/piptools/scripts/sync.py b/piptools/scripts/sync.py
index 076c965..40c086a 100755
--- a/piptools/scripts/sync.py
+++ b/piptools/scripts/sync.py
@@ -15,6 +15,12 @@ DEFAULT_REQUIREMENTS_FILE = "requirements.txt"
 
 @click.command()
 @click.version_option()
+@click.option(
+    "-a",
+    "--ask",
+    is_flag=True,
+    help="Show what would happen, then ask whether to continue",
+)
 @click.option(
     "-n",
     "--dry-run",
@@ -63,6 +69,7 @@ DEFAULT_REQUIREMENTS_FILE = "requirements.txt"
 )
 @click.argument("src_files", required=False, type=click.Path(exists=True), nargs=-1)
 def cli(
+    ask,
     dry_run,
     force,
     find_links,
@@ -137,5 +144,6 @@ def cli(
             verbose=(not quiet),
             dry_run=dry_run,
             install_flags=install_flags,
+            ask=ask,
         )
     )
diff --git a/piptools/sync.py b/piptools/sync.py
index a53b1dc..00b1ae8 100644
--- a/piptools/sync.py
+++ b/piptools/sync.py
@@ -144,7 +144,14 @@ def diff(compiled_requirements, installed_dists):
     return (to_install, to_uninstall)
 
 
-def sync(to_install, to_uninstall, verbose=False, dry_run=False, install_flags=None):
+def sync(
+    to_install,
+    to_uninstall,
+    verbose=False,
+    dry_run=False,
+    install_flags=None,
+    ask=False,
+):
     """
     Install and uninstalls the given sets of modules.
     """
@@ -157,26 +164,34 @@ def sync(to_install, to_uninstall, verbose=False, dry_run=False, install_flags=N
     if not verbose:
         pip_flags += ["-q"]
 
-    if to_uninstall:
-        if dry_run:
+    if ask:
+        dry_run = True
+
+    if dry_run:
+        if to_uninstall:
             click.echo("Would uninstall:")
             for pkg in to_uninstall:
                 click.echo("  {}".format(pkg))
-        else:
+
+        if to_install:
+            click.echo("Would install:")
+            for ireq in to_install:
+                click.echo("  {}".format(format_requirement(ireq)))
+
+    if ask and click.confirm("Would you like to proceed with these changes?"):
+        dry_run = False
+
+    if not dry_run:
+        if to_uninstall:
             check_call(  # nosec
                 [sys.executable, "-m", "pip", "uninstall", "-y"]
                 + pip_flags
                 + sorted(to_uninstall)
             )
 
-    if to_install:
-        if install_flags is None:
-            install_flags = []
-        if dry_run:
-            click.echo("Would install:")
-            for ireq in to_install:
-                click.echo("  {}".format(format_requirement(ireq)))
-        else:
+        if to_install:
+            if install_flags is None:
+                install_flags = []
             # prepare requirement lines
             req_lines = []
             for ireq in sorted(to_install, key=key_from_ireq):


Test patch:
diff --git a/tests/test_cli_sync.py b/tests/test_cli_sync.py
index b038534..fe54594 100644
--- a/tests/test_cli_sync.py
+++ b/tests/test_cli_sync.py
@@ -147,3 +147,30 @@ def test_pip_install_flags(check_call, cli_flags, expected_install_flags, runner
     assert [args[6:] for args in call_args if args[3] == "install"] == [
         expected_install_flags
     ]
+
+
+@mock.patch("piptools.sync.check_call")
+def test_sync_ask_declined(check_call, runner):
+    """
+    Make sure nothing is installed if the confirmation is declined
+    """
+    with open("requirements.txt", "w") as req_in:
+        req_in.write("small-fake-a==1.10.0")
+
+    runner.invoke(cli, ["--ask"], input="n\n")
+
+    check_call.assert_not_called()
+
+
+@mock.patch("piptools.sync.check_call")
+def test_sync_ask_accepted(check_call, runner):
+    """
+    Make sure pip is called when the confirmation is accepted (even if
+    --dry-run is given)
+    """
+    with open("requirements.txt", "w") as req_in:
+        req_in.write("small-fake-a==1.10.0")
+
+    runner.invoke(cli, ["--ask", "--dry-run"], input="y\n")
+
+    assert check_call.call_count == 2
diff --git a/tests/test_sync.py b/tests/test_sync.py
index e690a7b..3f2237f 100644
--- a/tests/test_sync.py
+++ b/tests/test_sync.py
@@ -403,39 +403,81 @@ def test_sync_verbose(check_call, from_line):
         assert "-q" not in check_call_args
 
 
+@pytest.mark.parametrize(
+    ("to_install", "to_uninstall", "expected_message"),
+    [
+        ({"django==1.8", "click==4.0"}, set(), "Would install:"),
+        (set(), {"django==1.8", "click==4.0"}, "Would uninstall:"),
+    ],
+)
 @mock.patch("piptools.sync.click.echo")
-def test_sync_dry_run_would_install(echo, from_line):
+def test_sync_dry_run(echo, from_line, to_install, to_uninstall, expected_message):
     """
-    Sync with --dry-run option prints what's is going to be installed.
+    Sync with --dry-run option prints what's is going to be installed/uninstalled.
     """
-    to_install = {from_line("django==1.8"), from_line("click==4.0")}
+    to_install = set(from_line(pkg) for pkg in to_install)
+    to_uninstall = set(from_line(pkg) for pkg in to_uninstall)
 
-    sync(to_install, set(), dry_run=True)
+    sync(to_install, to_uninstall, dry_run=True)
 
     expected_calls = [
-        mock.call("Would install:"),
+        mock.call(expected_message),
         mock.call("  django==1.8"),
         mock.call("  click==4.0"),
     ]
     echo.assert_has_calls(expected_calls, any_order=True)
 
 
+@pytest.mark.parametrize(
+    ("to_install", "to_uninstall", "expected_message"),
+    [
+        ({"django==1.8", "click==4.0"}, set(), "Would install:"),
+        (set(), {"django==1.8", "click==4.0"}, "Would uninstall:"),
+    ],
+)
+@mock.patch("piptools.sync.check_call")
+@mock.patch("piptools.sync.click.confirm")
 @mock.patch("piptools.sync.click.echo")
-def test_sync_dry_run_would_uninstall(echo, from_line):
+def test_sync_ask_declined(
+    echo, confirm, check_call, from_line, to_install, to_uninstall, expected_message
+):
     """
-    Sync with --dry-run option prints what is going to be uninstalled.
+    Sync with --ask option does a dry run if the user declines
     """
-    to_uninstall = {from_line("django==1.8"), from_line("click==4.0")}
+    confirm.return_value = False
+
+    to_install = set(from_line(pkg) for pkg in to_install)
+    to_uninstall = set(from_line(pkg) for pkg in to_uninstall)
 
-    sync(set(), to_uninstall, dry_run=True)
+    sync(to_install, to_uninstall, ask=True)
 
     expected_calls = [
-        mock.call("Would uninstall:"),
+        mock.call(expected_message),
         mock.call("  django==1.8"),
         mock.call("  click==4.0"),
     ]
     echo.assert_has_calls(expected_calls, any_order=True)
 
+    confirm.assert_called_once_with("Would you like to proceed with these changes?")
+    check_call.assert_not_called()
+
+
+@pytest.mark.parametrize("dry_run", [True, False])
+@mock.patch("piptools.sync.click.confirm")
+@mock.patch("piptools.sync.check_call")
+def test_sync_ask_accepted(check_call, confirm, from_line, dry_run):
+    """
+    pip should be called as normal when the user confirms, even with dry_run
+    """
+    confirm.return_value = True
+
+    sync(
+        {from_line("django==1.8")}, {from_line("click==4.0")}, ask=True, dry_run=dry_run
+    )
+    assert check_call.call_count == 2
+
+    confirm.assert_called_once_with("Would you like to proceed with these changes?")
+
 
 @mock.patch("piptools.sync.check_call")
 def test_sync_uninstall_pip_command(check_call):