From 917fd3d2c8ebaf197ca364c0f278e033cfe24683 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Fri, 26 Nov 2021 12:02:00 +0000 Subject: [PATCH] Review feedback and add checks to suite.rc and flow.cylc file logic. --- CHANGES.md | 1 + cylc/flow/workflow_files.py | 33 ++++++++++++++++---- tests/unit/test_workflow_files.py | 52 ++++++++++++++++++++++++++++++- 3 files changed, 79 insertions(+), 7 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e79d83a1aaf..e5b46b0fd35 100644 --- a/CHANGES.md +++ b/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 (Released 2021-11-10)__ diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index 9cf9b6e467e..c5ba83c1867 100644 --- a/cylc/flow/workflow_files.py +++ b/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( diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index c72917cf95a..af3893674ea 100644 --- a/tests/unit/test_workflow_files.py +++ b/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', [