Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow run n and run x as flow names #4526

Merged
merged 7 commits into from
Nov 29, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ First Release Candidate for Cylc 8.

### Enhancements

[#4526](https://github.com/cylc/cylc-flow/pull/4526) - Prevent runN and run\d+
being allowed as installation target names.

[#4442](https://github.com/cylc/cylc-flow/pull/4442) - Prevent installation
of workflows inside other installed workflows.

Expand Down
9 changes: 7 additions & 2 deletions cylc/flow/workflow_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -1311,7 +1311,7 @@ def _parse_src_reg(reg: Path, cur_dir_only: bool = False) -> Tuple[Path, Path]:
return (reg, abs_path)


def validate_workflow_name(name: str) -> None:
def validate_workflow_name(name: str, runNcheck=False) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this runNcheck argument?

Copy link
Member Author

@wxtim wxtim Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other commands that use validate_workflow_name can (sometimes should) be passed a name which ends in runN or runX:

Consider cylc clean /home/me/cylc-run/my-workflow/runN. Cylc clean uses validate workflow, and in that case it certainly doesn't want to claim that /home/me/cylc-run/my-workflow/runN is not valid.

Cylc play is the other use case.

"""Check workflow name is valid and not an absolute path.

Raise WorkflowFilesError if not valid.
Expand All @@ -1331,6 +1331,11 @@ def validate_workflow_name(name: str) -> None:
"Workflow name cannot be a path that points to the cylc-run "
"directory or above"
)
if runNcheck and re.findall(r'^run(N|\d+)$', Path(name).name):
wxtim marked this conversation as resolved.
Show resolved Hide resolved
raise WorkflowFilesError(
"Workflow name cannot be a folder called 'runN' or "
wxtim marked this conversation as resolved.
Show resolved Hide resolved
"'run<number>'."
)


def infer_latest_run(
Expand Down Expand Up @@ -1626,7 +1631,7 @@ def install_workflow(
source = Path(expand_path(source))
if not workflow_name:
workflow_name = source.name
validate_workflow_name(workflow_name)
validate_workflow_name(workflow_name, runNcheck=True)
if run_name in WorkflowFiles.RESERVED_NAMES:
raise WorkflowFilesError(f'Run name cannot be "{run_name}".')
if run_name is not None and len(Path(run_name).parts) != 1:
Expand Down
12 changes: 10 additions & 2 deletions tests/unit/test_workflow_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,17 @@ def test_check_nested_dirs_install_dirs(
('$HOME/alone', WorkflowFilesError, "invalid workflow name"),
('./foo', WorkflowFilesError, "invalid workflow name"),
('meow/..', WorkflowFilesError,
"cannot be a path that points to the cylc-run directory or above")]
"cannot be a path that points to the cylc-run directory or above"),
('run6', WorkflowFilesError, "cannot be a folder called 'runN'"),
('e/run6', WorkflowFilesError, "cannot be a folder called 'runN'"),
('runN', WorkflowFilesError, "cannot be a folder called 'runN'"),
('e/runN', WorkflowFilesError, "cannot be a folder called 'runN'")]
)
def test_validate_workflow_name(reg, expected_err, expected_msg):
if expected_err:
with pytest.raises(expected_err) as exc:
workflow_files.validate_workflow_name(reg)
runNcheck = 'cannot be a folder called' in expected_msg
workflow_files.validate_workflow_name(reg, runNcheck=runNcheck)
if expected_msg:
assert expected_msg in str(exc.value)
else:
Expand Down Expand Up @@ -1628,6 +1633,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',
[
Expand Down Expand Up @@ -1809,6 +1815,7 @@ 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."""
tmp_path.joinpath(WorkflowFiles.FLOW_FILE).touch()
Expand All @@ -1824,6 +1831,7 @@ def test_detect_both_flow_and_suite(tmp_path):
"#backward-compatibility"
)


@pytest.mark.parametrize(
'flow_file_target, suiterc_exists, err, expected_file',
[
Expand Down