Review feedback and add checks to suite.rc and flow.cylc file logic.

CHANGES.md

@@ -56,6 +56,7 @@ First release candidate of Cylc 8.
 
 [#4506](https://github.com/cylc/cylc-flow/pull/4506) -
 Cylc no longer creates a `flow.cylc` symlink to a `suite.rc` file.
+This only affects you if you have used a prior Cylc 8 pre-release.
 
 -------------------------------------------------------------------------------
 ## __cylc-8.0b3 (<span actions:bind='release-date'>Released 2021-11-10</span>)__

cylc/flow/workflow_files.py

@@ -539,7 +539,7 @@ def get_contact_file(reg):
         get_workflow_srv_dir(reg), WorkflowFiles.Service.CONTACT)
 
 
-def get_flow_file(reg: str) -> str:
+def get_flow_file(reg: str) -> Path:
     """Return the path of a workflow's flow.cylc file.
 
     Creates a flow.cylc symlink to suite.rc if only suite.rc exists.
@@ -1697,25 +1697,46 @@ def get_run_dir_info(
     return relink, run_num, rundir
 
 
-def detect_both_flow_and_suite(path):
+def detect_both_flow_and_suite(path: Path) -> None:
     """Detects if both suite.rc and flow.cylc are in directory.
 
     Permits flow.cylc to be a symlink.
+    Return true if present, raises error if flow.cylc path sent is a forbidden
+    symlink.
     Raises:
         WorkflowFilesError: If both flow.cylc and suite.rc are in directory
     """
+    flow_cylc = None
     msg = (f"Both {WorkflowFiles.FLOW_FILE} and {WorkflowFiles.SUITE_RC} "
            f"files are present in {path}. Please remove one and"
            " try again. For more information visit: https://cylc.github.io/"
            "cylc-doc/latest/html/7-to-8/summary.html#backward-compatibility")
     if path.resolve().name == WorkflowFiles.SUITE_RC:
         flow_cylc = path.parent / WorkflowFiles.FLOW_FILE
-        if flow_cylc.is_file() and not flow_cylc.is_symlink():
-            raise WorkflowFilesError(msg)
     elif (path / WorkflowFiles.SUITE_RC).is_file():
         flow_cylc = path / WorkflowFiles.FLOW_FILE
-        if flow_cylc.is_file() and not flow_cylc.is_symlink():
-            raise WorkflowFilesError(msg)
+    if flow_cylc and flow_cylc.is_file() and is_forbidden(flow_cylc):
+        raise WorkflowFilesError(msg)
+
+
+def is_forbidden(flow_file: Path) -> bool:
+    """Returns True for a forbidden file structure scenario.
+
+    Forbidden criteria:
+        A symlink elsewhere on file system but suite.rc also exists in the
+        directory.
+        flow.cylc and suite.rc in same directory but no symlink
+    Args:
+        flow_file : Absolute Path to the flow.cylc file
+    """
+    if not flow_file.is_symlink():
+        if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists():
+            return True
+        return False
+    link = flow_file.resolve()
+    if Path(link).parent == flow_file.parent:  # link points within dir
+        return False
+    return True
 
 
 def detect_flow_exists(

tests/unit/test_workflow_files.py

@@ -50,6 +50,7 @@
     glob_in_run_dir,
     infer_latest_run,
     init_clean,
+    is_forbidden,
     is_installed,
     parse_cli_sym_dirs,
     parse_reg,
@@ -1575,6 +1576,7 @@ def mocked_remote_clean_cmd_side_effect(reg, platform, rm_dirs, timeout):
         for p_name in failed_platforms:
             assert f"{p_name} - {PlatformError.MSG_TIDY}" in caplog.text
 
+
 @pytest.mark.parametrize(
     'rm_dirs, expected_args',
     [
@@ -1756,11 +1758,14 @@ def test_check_flow_file(
     else:
         assert check_flow_file(tmp_path) == tmp_path.joinpath(expected_file)
 
+
 def test_detect_both_flow_and_suite(tmp_path):
-    """Test flow.cylc and suite.rc together in dir raises error."""
+    """Test flow.cylc and suite.rc (as files) together in dir raises error."""
     tmp_path.joinpath(WorkflowFiles.FLOW_FILE).touch()
     tmp_path.joinpath(WorkflowFiles.SUITE_RC).touch()
 
+    forbidden = is_forbidden(tmp_path / WorkflowFiles.FLOW_FILE)
+    assert forbidden is True
     with pytest.raises(WorkflowFilesError) as exc:
         detect_both_flow_and_suite(tmp_path)
         assert str(exc.value) == (
@@ -1772,6 +1777,51 @@ def test_detect_both_flow_and_suite(tmp_path):
         )
 
 
+def test_detect_both_flow_and_suite_symlinked(tmp_path):
+    """Test flow.cylc symlinked to suite.rc together in dir is permitted."""
+    Path(tmp_path)
+    (tmp_path / WorkflowFiles.SUITE_RC).touch()
+    flow_file = tmp_path.joinpath(WorkflowFiles.FLOW_FILE)
+    flow_file.symlink_to(WorkflowFiles.SUITE_RC)
+    try:
+        detect_both_flow_and_suite(tmp_path)
+    except WorkflowFilesError:
+        pytest.fail("Unexpected WorkflowFilesError")
+
+
+def test_flow_symlinked_elsewhere_and_suite_present(tmp_path):
+    """flow.cylc symlinked to suite.rc elsewhere, and suite.rc in dir raises"""
+    tmp_path = Path(tmp_path)
+    tmp_path.joinpath('some_other_dir').mkdir(exist_ok=True)
+    suite_file = tmp_path.joinpath('some_other_dir', WorkflowFiles.SUITE_RC)
+    suite_file.touch()
+    run_dir = tmp_path.joinpath('run_dir')
+    run_dir.mkdir(exist_ok=True)
+    flow_file = (run_dir / WorkflowFiles.FLOW_FILE)
+    flow_file.symlink_to(suite_file)
+    (run_dir / WorkflowFiles.SUITE_RC).touch()
+    forbidden = is_forbidden(flow_file)
+    assert forbidden is True
+    with pytest.raises(WorkflowFilesError) as exc:
+        detect_both_flow_and_suite(run_dir)
+        assert str(exc.value) == (
+            "Both flow.cylc and suite.rc files are present in the "
+            "source directory. Please remove one and try again. "
+            "For more information visit: "
+            "https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html"
+            "#backward-compatibility"
+        )
+
+
+def test_is_forbidden_symlink_returns_false_for_non_symlink(tmp_path):
+    """Test sending a non symlink path is not marked as forbidden"""
+    Path(tmp_path)
+    flow_file = (tmp_path / WorkflowFiles.FLOW_FILE)
+    flow_file.touch()
+    forbidden = is_forbidden(Path(flow_file))
+    assert forbidden is False
+
+
 @pytest.mark.parametrize(
     'symlink_dirs, err_msg, expected',
     [