Merge pull request #6658 from MetRonnie/rsync

Fix reinstall not detecting changed permissions by itself

Author: oliver-sanders

changes.d/6658.fix.md

@@ -0,0 +1 @@
+Fixed `cylc reinstall` not picking up file permissions changes.

cylc/flow/install.py

@@ -213,19 +213,16 @@ def reinstall_workflow(
         dry_run=dry_run,
     )
 
-    # Add '+++' to -out-format to mark lines passed through formatter.
-    rsync_cmd.append('--out-format=+++%o %n%L+++')
+    rsync_cmd.append('--out-format=%i %o %n%L')
+    # %i: itemized changes - needed for rsync to report files with
+    # changed permissions
 
     # Run rsync command:
     reinstall_log.info(cli_format(rsync_cmd))
     LOG.debug(cli_format(rsync_cmd))
     proc = Popen(rsync_cmd, stdout=PIPE, stderr=PIPE, text=True)  # nosec
     # * command is constructed via internal interface
-    stdout, stderr = proc.communicate()
-
-    # Strip unwanted output.
-    stdout = ('\n'.join(re.findall(r'\+\+\+(.*)\+\+\+', stdout))).strip()
-    stderr = stderr.strip()
+    stdout, stderr = (i.strip() for i in proc.communicate())
 
     if proc.returncode != 0:
         raise WorkflowFilesError(

cylc/flow/remote.py

@@ -183,6 +183,9 @@ def get_includes_to_rsync(rsync_includes=None):
     '--out-format=%o %n%L',
     '--no-t'
 ]
+# %o: the operation (send or del.)
+# %n: filename
+# %L: "-> symlink_target" if applicable
 
 DEFAULT_INCLUDES = [
     '/ana/***',  # Rose ana analysis modules

cylc/flow/scripts/reinstall.py

@@ -68,33 +68,43 @@
   "cylc install" even if present in the source directory.
 """
 
+from functools import partial
 from pathlib import Path
+import re
 import sys
-from typing import Optional, TYPE_CHECKING, List, Callable
-from functools import partial
+from typing import (
+    TYPE_CHECKING,
+    Callable,
+    List,
+    Optional,
+)
 
 from ansimarkup import parse as cparse
 
 from cylc.flow.exceptions import (
     ServiceFileError,
     WorkflowFilesError,
 )
-from cylc.flow.install import (
-    reinstall_workflow,
-)
+import cylc.flow.flags
+from cylc.flow.install import reinstall_workflow
 from cylc.flow.network.multi import call_multi
 from cylc.flow.option_parsers import (
+    ID_MULTI_ARG_DOC,
     CylcOptionParser as COP,
     OptionSettings,
-    ID_MULTI_ARG_DOC
 )
 from cylc.flow.pathutil import get_workflow_run_dir
 from cylc.flow.plugins import run_plugins_async
+from cylc.flow.terminal import (
+    DIM,
+    cli_function,
+    is_terminal,
+)
 from cylc.flow.workflow_files import (
     get_workflow_source_dir,
     load_contact_file,
 )
-from cylc.flow.terminal import cli_function, DIM, is_terminal
+
 
 if TYPE_CHECKING:
     from optparse import Values
@@ -269,9 +279,8 @@ async def reinstall(
         update anything, else returns True.
 
     """
-    # run pre_configure plugins
     if not dry_run:
-        # don't run plugins in dry-mode
+        # run pre_configure plugins
         async for _entry_point, _plugin_result in run_plugins_async(
             'cylc.pre_configure',
             srcdir=src_dir,
@@ -287,18 +296,17 @@ async def reinstall(
         dry_run=dry_run,
     )
 
-    # display changes
     if dry_run:
-        if not stdout or stdout == 'send ./':
-            # no rsync output == no changes => exit
+        # display changes
+        changes = format_reinstall_output(stdout)
+        if not changes:
             return False
 
         # display rsync output
-        write('\n'.join(format_rsync_out(stdout)), file=sys.stderr)
+        write('\n'.join(changes), file=sys.stdout)
 
-    # run post_install plugins
     if not dry_run:
-        # don't run plugins in dry-mode
+        # run post_install plugins
         async for _entry_point, _plugin_result in run_plugins_async(
             'cylc.post_install',
             srcdir=src_dir,
@@ -311,15 +319,15 @@ async def reinstall(
     return True
 
 
-def format_rsync_out(out: str) -> List[str]:
+def format_reinstall_output(out: str) -> List[str]:
     r"""Format rsync stdout for presenting to users.
 
     Note: Output formats of different rsync implementations may differ so keep
           this code simple and robust.
 
     Example:
-        >>> format_rsync_out(
-        ...     'send foo\ndel. bar\nbaz'
+        >>> format_reinstall_output(
+        ...     '>f+++++++++ send foo\n*deleting   del. bar\nbaz'
         ...     '\ncannot delete non-empty directory: opt'
         ... ) == [
         ...     cparse('<green>send foo</green>'),
@@ -331,20 +339,23 @@ def format_rsync_out(out: str) -> List[str]:
     """
     lines = []
     for line in out.splitlines():
-        if line[0:4] == 'send':
-            # file added or updated
-            lines.append(cparse(f'<green>{line}</green>'))
-        elif line[0:4] == 'del.':
-            # file deleted
-            lines.append(cparse(f'<red>{line}</red>'))
-        elif line == 'cannot delete non-empty directory: opt':
-            # These "cannot delete non-empty directory" messages can arise
-            # as a result of excluding files within sub-directories.
-            # This opt dir message is likely to occur when a rose-suit.conf
+        if not line or line.startswith('cannot delete non-empty directory:'):
+            # This can arise as a result of deleting a subdir when we are
+            # excluding files within the subdir.
+            # This is likely to occur for the opt dir when a rose-suite.conf
             # file is present.
+            # Skip this line as nothing will happen to this dir.
             continue
+        match = re.match(r'^(.{11}) (send|del\.) (.*)$', line)
+        if match:
+            summary, operation, file = match.groups()
+            color = 'green' if operation == 'send' else 'red'
+            formatted_line = f"<{color}>{operation} {file}</{color}>"
+            if cylc.flow.flags.verbosity > 0:
+                formatted_line = f"<{DIM}>{summary}</{DIM}> {formatted_line}"
+            lines.append(cparse(formatted_line))
         else:
-            # other uncategorised log line
+            # shouldn't happen; tolerate unknown rsync implementation?
             lines.append(line)
     return lines
 

tests/integration/test_reinstall.py

@@ -19,28 +19,23 @@
 import asyncio
 from contextlib import asynccontextmanager
 from pathlib import Path
+from secrets import token_hex
 from types import SimpleNamespace
-from uuid import uuid1
 
 import pytest
 
-from cylc.flow.exceptions import (
-    WorkflowFilesError,
-)
-from cylc.flow.install import (
-    reinstall_workflow,
-)
+from cylc.flow.exceptions import WorkflowFilesError
+from cylc.flow.install import reinstall_workflow
 from cylc.flow.option_parsers import Options
 from cylc.flow.scripts.reinstall import (
     get_option_parser as reinstall_gop,
     reinstall_cli,
 )
-from cylc.flow.workflow_files import (
-    WorkflowFiles,
-)
+from cylc.flow.workflow_files import WorkflowFiles
 
 from .utils.entry_points import EntryPointWrapper
 
+
 ReInstallOptions = Options(reinstall_gop())
 
 # cli opts
@@ -66,17 +61,30 @@ def non_interactive(monkeypatch):
     )
 
 
+@pytest.fixture
+def answer_prompt(monkeypatch: pytest.MonkeyPatch):
+    """Answer reinstall prompt."""
+
+    def inner(answer: str):
+        monkeypatch.setattr(
+            'cylc.flow.scripts.reinstall._input', lambda x: answer
+        )
+
+    return inner
+
+
 @pytest.fixture
 def one_src(tmp_path):
-    src_dir = tmp_path
+    src_dir = tmp_path / 'src'
+    src_dir.mkdir()
     (src_dir / 'flow.cylc').touch()
     (src_dir / 'rose-suite.conf').touch()
     return SimpleNamespace(path=src_dir)
 
 
 @pytest.fixture
 def one_run(one_src, test_dir, run_dir):
-    w_run_dir = test_dir / str(uuid1())
+    w_run_dir = test_dir / token_hex(4)
     w_run_dir.mkdir()
     (w_run_dir / 'flow.cylc').touch()
     (w_run_dir / 'rose-suite.conf').touch()
@@ -151,7 +159,7 @@ async def test_interactive(
     capsys,
     capcall,
     interactive,
-    monkeypatch
+    answer_prompt
 ):
     """It should perform a dry-run and prompt in interactive mode."""
     # capture reinstall calls
@@ -162,11 +170,7 @@ async def test_interactive(
     # give it something to reinstall
     (one_src.path / 'a').touch()
 
-    # reinstall answering "no" to any prompt
-    monkeypatch.setattr(
-        'cylc.flow.scripts.reinstall._input',
-        lambda x: 'n'
-    )
+    answer_prompt('n')
     assert (
         await reinstall_cli(opts=ReInstallOptions(), workflow_id=one_run.id)
         is False
@@ -177,11 +181,7 @@ async def test_interactive(
     assert 'Reinstall canceled, no changes made.' in capsys.readouterr().out
     reinstall_calls.clear()
 
-    # reinstall answering "yes" to any prompt
-    monkeypatch.setattr(
-        'cylc.flow.scripts.reinstall._input',
-        lambda x: 'y'
-    )
+    answer_prompt('y')
     assert await reinstall_cli(opts=ReInstallOptions(), workflow_id=one_run.id)
 
     # two rsync calls should have been made (i.e. the --dry-run and the real)
@@ -241,7 +241,7 @@ async def test_rsync_stuff(one_src, one_run, capsys, non_interactive):
 
 
 async def test_rose_warning(
-    one_src, one_run, capsys, interactive, monkeypatch
+    one_src, one_run, capsys, interactive, answer_prompt
 ):
     """It should warn that Rose installed files will be deleted.
 
@@ -252,11 +252,7 @@ async def test_rose_warning(
         'Files created by Rose file installation will show as deleted'
     )
 
-    # reinstall answering "no" to any prompt
-    monkeypatch.setattr(
-        'cylc.flow.scripts.reinstall._input',
-        lambda x: 'n'
-    )
+    answer_prompt('n')
     (one_src.path / 'a').touch()  # give it something to install
 
     # reinstall (with rose-suite.conf file)
@@ -311,6 +307,35 @@ async def test_rsync_fail(one_src, one_run, mock_glbl_cfg, non_interactive):
     assert 'An error occurred reinstalling' in str(exc_ctx.value)
 
 
+async def test_permissions_change(
+    one_src,
+    one_run,
+    interactive,
+    answer_prompt,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture,
+):
+    """It detects permissions changes."""
+    # Add script file:
+    script_path: Path = one_src.path / 'myscript'
+    script_path.touch()
+    await reinstall_cli(
+        opts=ReInstallOptions(skip_interactive=True), workflow_id=one_run.id
+    )
+    assert (one_run.path / 'myscript').is_file()
+    capsys.readouterr()  # clears capsys
+
+    # Change permissions (e.g. user forgot to make it executable before)
+    script_path.chmod(0o777)
+    # Answer "no" to reinstall prompt (we just want to test dry run)
+    answer_prompt('n')
+    await reinstall_cli(
+        opts=ReInstallOptions(), workflow_id=one_run.id
+    )
+    out, _ = capsys.readouterr()
+    assert "send myscript" in out
+
+
 @pytest.fixture
 def my_install_plugin(monkeypatch):
     """This configures a single post_install plugin.

tests/unit/scripts/test_reinstall.py

@@ -0,0 +1,56 @@
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from textwrap import dedent
+
+from ansimarkup import parse as cparse
+import pytest
+
+from cylc.flow.scripts.reinstall import format_reinstall_output
+from cylc.flow.terminal import DIM
+
+
+@pytest.mark.parametrize('verbose', [True, False])
+def test_format_reinstall_output(verbose, monkeypatch: pytest.MonkeyPatch):
+    """It should:
+    - colorize the output
+    - remove the itemized changes summary if not in verbose mode
+    - remove the "cannot delete non-empty directory" message
+    """
+    output = dedent("""
+        *deleting   del. Cloud.jpg
+        >f+++++++++ send cloud.jpg
+        .f...p..... send foo
+        >fcsTp..... send bar
+        cannot delete non-empty directory: scarf
+        >f+++++++++ send meow.txt
+        cL+++++++++ send garage -> foo
+    """).strip()
+    expected = [
+        f"<{DIM}>*deleting  </{DIM}> <red>del. Cloud.jpg</red>",
+        f"<{DIM}>>f+++++++++</{DIM}> <green>send cloud.jpg</green>",
+        f"<{DIM}>.f...p.....</{DIM}> <green>send foo</green>",
+        f"<{DIM}>>fcsTp.....</{DIM}> <green>send bar</green>",
+        f"<{DIM}>>f+++++++++</{DIM}> <green>send meow.txt</green>",
+        f"<{DIM}>cL+++++++++</{DIM}> <green>send garage -> foo</green>",
+    ]
+    if verbose:
+        monkeypatch.setattr('cylc.flow.flags.verbosity', 1)
+    else:
+        # itemized changes summary should not be in output
+        shift = len(f'<{DIM}></{DIM}> ') + 11
+        expected = [i[shift:] for i in expected]
+    assert format_reinstall_output(output) == [cparse(i) for i in expected]