Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Remove the suite.rc flow.cylc symlink #4506
Remove the suite.rc flow.cylc symlink #4506
Changes from 6 commits
9f401fe
c14db21
a4354b6
d638b3e
e42f548
0beeac4
a49283c
90f2693
8d55298
97fab79
6e4895c
5484cb6
46d7737
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this desirable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliver-sanders, I believe you said we should not stop users from manually making a symlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check that the symlink points to the right suite.rc, and raise an error if not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolving this as there is a use case (at the MO) for users to source workflow from outside the run directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't see that, thanks @MetRonnie, mustn't have refreshed the page before posting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow
which is equivalent to having distinct
flow.cylc
andsuite.rc
files in the same run dir.Perhaps it should be something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case (I think) is covered by the following condition:
if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists():
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aresolve()
on the end to hit the false.Will go with your suggestion (adding a resolve()), thanks @MetRonnie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.