Remove the suite.rc flow.cylc symlink #4506

datamel · 2021-11-10T18:18:33Z

Removes the flow.cylc symlink created when using a suite.rc file. This also enabled a refactor of duplicate logic from #4497.
These changes close #4500

Milestone is 8.0rc1 to give time for any unforeseen problems to surface.

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.py and
conda-environment.yml.
Already covered by existing tests (removed now redundant tests).
Appropriate change log entry included.
No documentation update required (I can not find reference in the docs to this symlink being created).

wxtim · 2021-11-15T13:28:51Z

cylc/flow/workflow_files.py

@@ -1700,11 +1699,13 @@ def get_run_dir_info(

 def detect_both_flow_and_suite(path):
    """Detects if both suite.rc and flow.cylc are in directory.
+
+    Permits flow.cylc to be a symlink.


Is this desirable?

@oliver-sanders, I believe you said we should not stop users from manually making a symlink.

Shouldn't we check that the symlink points to the right suite.rc, and raise an error if not?

If there is a flow.cylc there it will use that, regardless of if there is a suite.rc file there. Do we want to forbid users from symlinking other places? I can't think of a reason to forbid, I am probably missing something! I think so long as it is a valid workflow (which will be picked up with other checks) then do we really care how they are sourcing the workflow, it is a bit messy but that is the user's choice?

It sounds reasonable to me that users could have a flow.cylc that is a symlink to either its neighbour suite.rc or somewhere else. But if they have a flow.cylc that symlinks to somewhere else and a suite.rc in there, that sounds to me like it should fail in the same way as a normal flow.cylc file and a suite.rc in there.

TLDR:

ok_but_a_bit_dodgy `-- flow.cylc -> ~/something.cylc ok |-- flow.cylc -> suite.rc # or flow.cylc -> ~/cylc-run/ok/suite.rc? `-- suite.rc bad |-- flow.cylc -> ~/something.cylc `-- suite.rc

Resolving this as there is a use case (at the MO) for users to source workflow from outside the run directory.

Sorry Mel, not sure if you saw my latest comment above #4506 (comment). I think it is similar to what Oliver said (except he said we don't have to worry about the middle case)

Ah I didn't see that, thanks @MetRonnie, mustn't have refreshed the page before posting.

wxtim

Read -looks sensible.
Played with.

I had one question about the functionality - do we want to allow the symlink to pass?

CHANGES.md

cylc/flow/workflow_files.py

MetRonnie · 2021-11-15T17:23:12Z

cylc/flow/workflow_files.py

@@ -1700,11 +1699,13 @@ def get_run_dir_info(

 def detect_both_flow_and_suite(path):
    """Detects if both suite.rc and flow.cylc are in directory.
+
+    Permits flow.cylc to be a symlink.


Shouldn't we check that the symlink points to the right suite.rc, and raise an error if not?

cylc/flow/workflow_files.py

tests/functional/cylc-install/00-simple.t

cylc/flow/workflow_files.py

tests/unit/test_workflow_files.py

MetRonnie · 2021-11-26T12:55:32Z

tests/unit/test_workflow_files.py

-    if log_msg:
-        assert log_msg in caplog.messages
+def test_is_forbidden_symlink_returns_false_for_non_symlink(tmp_path):
+    """Test sending a non symlink path is not marked as forbidden"""


Shouldn't a lone symlink also be not marked as forbidden?

Yes, thanks @MetRonnie. Small adjustment and test added. I hope all bases are covered this time!

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

add test for deprecation message cylc install

MetRonnie · 2021-11-29T18:15:28Z

cylc/flow/workflow_files.py

+    link = flow_file.resolve()
+    if link.parent == flow_file.parent:


This would allow

myflow |-- something.cylc |-- flow.cylc -> something.cylc `-- suite.rc

which is equivalent to having distinct flow.cylc and suite.rc files in the same run dir.

Perhaps it should be something like

if flow_file.resolve() == flow_file.parent / WorkflowFiles.SUITE_RC:

This case (I think) is covered by the following condition: if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists():.

It would be, but it will return False before getting to that.

See my suggestion: https://github.com/cylc/cylc-flow/pull/4506/files#r760191324

Sorry for my confusion! All testing for me was raising the error....

The return False does not return before that (where it should!) because flow_file.parent needed a resolve() on the end to hit the false.
Will go with your suggestion (adding a resolve()), thanks @MetRonnie

Ok, coming back to this (confusion after your suggestion was added by Tim so it wasn't visible to me). This suggestion will forbid symlinking elsewhere.
Going to push a change which I think now covers all scenarios, based on your if flow_file.resolve() == flow_file.parent / WorkflowFiles.SUITE_RC: suggestion.

tests/unit/test_workflow_files.py

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

…low into remove-suite-flow-symlink

datamel · 2021-12-01T09:32:43Z

@wxtim I've made some logic changes since your approval, sorry! So requesting re-review.

cylc/flow/workflow_files.py

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

wxtim · 2021-12-02T10:25:47Z

tests/unit/test_workflow_files.py

-
-    if log_msg:
-        assert log_msg in caplog.messages
+def test_is_forbidden_symlink_returns_false_for_non_symlink(tmp_path):


Can we test some scenarios where is_forbidden → True?
Apart from Codecov highlighting it, I would expect there to be ~2 (sets of/parameters?) tests for a function returning a boolean.

There are currently 2 tests covering this scenario, is forbidden → True. I think that the two that are there are sufficient but let me know if you'd like more added. I'll have a look at why the codecov thinks otherwise.

MetRonnie

The code could probably be refactored to simplify it, but that can wait.

datamel added the small label Nov 10, 2021

datamel added this to the cylc-8.0rc1 milestone Nov 10, 2021

datamel self-assigned this Nov 10, 2021

datamel requested review from wxtim and MetRonnie November 15, 2021 11:55

wxtim reviewed Nov 15, 2021

View reviewed changes

wxtim approved these changes Nov 15, 2021

View reviewed changes

MetRonnie reviewed Nov 15, 2021

View reviewed changes

cylc/flow/workflow_files.py Show resolved Hide resolved

datamel force-pushed the remove-suite-flow-symlink branch from 917fd3d to b3dca10 Compare November 26, 2021 12:15

MetRonnie reviewed Nov 26, 2021

View reviewed changes

datamel force-pushed the remove-suite-flow-symlink branch 2 times, most recently from 88ca3aa to e366a38 Compare November 29, 2021 13:49

datamel and others added 5 commits November 29, 2021 16:45

Remove the suite.rc flow.cylc symlink

9f401fe

change log

c14db21

shellcheck

a4354b6

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

d638b3e

Update cylc/flow/workflow_files.py

e42f548

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

datamel force-pushed the remove-suite-flow-symlink branch from eb41b0f to a843267 Compare November 29, 2021 16:45

Add test and tweak logic to permit symlinks elsewhere for flow.cylc file

0beeac4

add test for deprecation message cylc install

datamel force-pushed the remove-suite-flow-symlink branch from a843267 to 0beeac4 Compare November 29, 2021 17:21

MetRonnie reviewed Nov 29, 2021

View reviewed changes

datamel and others added 4 commits November 30, 2021 09:53

Update tests/unit/test_workflow_files.py

a49283c

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

dedent test

90f2693

Merge branch 'remove-suite-flow-symlink' of github.com:datamel/cylc-f…

8d55298

…low into remove-suite-flow-symlink

Fix test

97fab79

datamel requested a review from MetRonnie December 1, 2021 09:31

datamel requested a review from wxtim December 1, 2021 09:32

MetRonnie reviewed Dec 1, 2021

View reviewed changes

cylc/flow/workflow_files.py Outdated Show resolved Hide resolved

Update cylc/flow/workflow_files.py

6e4895c

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

wxtim reviewed Dec 2, 2021

View reviewed changes

datamel added 2 commits December 2, 2021 11:16

Change logic comparing symlinked flow.cylc file.

5484cb6

logic fix

46d7737

MetRonnie approved these changes Dec 2, 2021

View reviewed changes

wxtim merged commit 06d78c2 into cylc:master Dec 3, 2021

MetRonnie mentioned this pull request Dec 6, 2021

Tidy + reinstate deleted workflow_files unit test #4544

Merged

hjoliver mentioned this pull request Dec 7, 2021

2021 Cylc Meetings cylc/cylc-admin#139

Closed

oliver-sanders mentioned this pull request Mar 25, 2022

scan: exclude Cylc 7 workflows from scan results #4771

Merged

7 tasks

datamel deleted the remove-suite-flow-symlink branch September 1, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the suite.rc flow.cylc symlink #4506

Remove the suite.rc flow.cylc symlink #4506

datamel commented Nov 10, 2021 •

edited

Loading

wxtim Nov 15, 2021

datamel Nov 15, 2021

MetRonnie Nov 15, 2021

datamel Nov 16, 2021 •

edited

Loading

MetRonnie Nov 16, 2021 •

edited

Loading

datamel Nov 16, 2021

MetRonnie Nov 16, 2021 •

edited

Loading

datamel Nov 16, 2021

wxtim left a comment

MetRonnie Nov 15, 2021

MetRonnie Nov 26, 2021 •

edited

Loading

datamel Nov 29, 2021

MetRonnie Nov 29, 2021

datamel Nov 30, 2021

MetRonnie Dec 1, 2021 •

edited

Loading

datamel Dec 2, 2021

datamel Dec 2, 2021

datamel commented Dec 1, 2021

wxtim Dec 2, 2021 •

edited

Loading

datamel Dec 2, 2021 •

edited

Loading

MetRonnie left a comment

		link = flow_file.resolve()
		if link.parent == flow_file.parent:

Remove the suite.rc flow.cylc symlink #4506

Remove the suite.rc flow.cylc symlink #4506

Conversation

datamel commented Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

MetRonnie Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel commented Dec 1, 2021

wxtim Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

datamel Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

datamel commented Nov 10, 2021 •

edited

Loading

datamel Nov 16, 2021 •

edited

Loading

MetRonnie Nov 16, 2021 •

edited

Loading

MetRonnie Nov 16, 2021 •

edited

Loading

MetRonnie Nov 26, 2021 •

edited

Loading

MetRonnie Dec 1, 2021 •

edited

Loading

wxtim Dec 2, 2021 •

edited

Loading

datamel Dec 2, 2021 •

edited

Loading