Symlink dirs cli #4250

datamel · 2021-06-08T12:51:55Z

Adds cli option (--symlink-dirs) for over-riding localhost symlink dirs in global config when executing cylc install.
Removes --no-symlink-dirs option, this is replaced by --symlink-dirs=None

Also moves symlink dirs into install section of config...(these changes close #4219)

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.

Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
(master branch) I have opened a documentation PR at symlink dirs cli docs cylc-doc#252

cylc/flow/workflow_files.py

hjoliver · 2021-06-09T21:47:20Z

Removes --no-symlink-dirs option, this is replaced by --symlink-dirs=None

I'm not sure about this. The original --no-symlink-dirs is pretty unambiguous and intuitive, but --symlink-dirs=None kind of exposes Python internals through the CLI and is less intuitive. Users would more likely guess --symlink-dirs="" (empty string), which does nothing.

hjoliver · 2021-06-09T21:50:26Z

I'm slight confused about how this works (from a quick test, without examining the code). If I set (e.g.) log and work symlinks for localhost in global.cylc, both symlinks get created by cylc install. If I then override log using cylc install --symlink-dirs, I find that cylc install creates only the overridden log symlink, and the global config work one gets created later by cylc play ??

oliver-sanders

Tested, looks good:

$ cylc config -i '[install][symlink dirs]'
[[[localhost]]]
    log = $HOME/cylc-log
    run = $HOME/cylc-runn
    share = 
    share/cycle = 
    work = 
$ cylc install one 
INSTALLED one/run1 from .../one
$ cylc install one --symlink-dirs=
INSTALLED one/run2 from .../one
$ cylc install one --symlink-dirs=log=$HOME/cylc-log
INSTALLED one/run3 from .../one
$ tree -l
.
├── _cylc-install
│   └── source -> .../one
│       └── suite.rc
├── run1 -> /cylc-runn/cylc-run/one/run1
│   ├── flow.cylc -> suite.rc
│   ├── log -> /cylc-log/cylc-run/one/run1/log
│   │   ├── install
│   │   │   └── 20210616T102225Z-install.log
│   │   └── version
│   │       ├── uncommitted.diff
│   │       └── vcs.conf
│   └── suite.rc
├── run2
│   ├── flow.cylc -> suite.rc
│   ├── log
│   │   ├── install
│   │   │   └── 20210616T102232Z-install.log
│   │   └── version
│   │       ├── uncommitted.diff
│   │       └── vcs.conf
│   └── suite.rc
├── run3
│   ├── flow.cylc -> suite.rc
│   ├── log -> /cylc-log/cylc-run/one/run3/log
│   │   ├── install
│   │   │   └── 20210616T102253Z-install.log
│   │   └── version
│   │       ├── uncommitted.diff
│   │       └── vcs.conf
│   └── suite.rc
└── runN -> run3  [recursive, not followed]

Tested again overriding run and work, the cylc play overriding has been addressed.

cylc/flow/pathutil.py

cylc/flow/workflow_files.py

datamel · 2021-06-16T21:31:41Z

I'm slight confused about how this works (from a quick test, without examining the code). If I set (e.g.) log and work symlinks for localhost in global.cylc, both symlinks get created by cylc install. If I then override log using cylc install --symlink-dirs, I find that cylc install creates only the overridden log symlink, and the global config work one gets created later by cylc play ??

Sorry about that @hjoliver chatted this through with the team this end and have implemented the following changes.

If cylc play-ing an installed flow, localhost symlink dirs will now be skipped.
Missing symlink dirs entered on the command line will not fill from global config.
--symlink-dirs="" is now the interface for skipping symlink dirs when installing flows.

The dropping of --no-symlink-dirs was an effort to keep the cli concise, rather than bloating with too many options.

Hope this is ok, happy to make changes if you wish.

oliver-sanders · 2021-06-17T08:27:15Z

The flake8 rules have just become a little stricter due to #4271, one small violation:

./cylc/flow/workflow_files.py:1383:9: SIM106 Handle error-cases first

Find out more here - https://github.com/MartinThoma/flake8-simplify#rules

hjoliver

Looks great, tests as working, just a few minor suggestions and questions.

cylc/flow/cfgspec/globalcfg.py

cylc/flow/workflow_files.py

MetRonnie

I think this is the PR you wanted me to check after #4237 was merged

cylc/flow/pathutil.py

MetRonnie · 2021-06-30T16:49:57Z

cylc/flow/pathutil.py

-    if path.exists():
-        # Trying to link to itself; no symlink needed
-        # (e.g. path's parent is symlink to target's parent)
-        return False


I think this is needed for the case of share and share/cycle in global.cylc pointing to the same place

[symlink dirs] share = ~/sym-share share/cycle = ~/sym-share

Line 226 has the the return false.

But this checks if the path exists after the target is created.

When the ~/cylc-run/foo/share -> ~/sym-share/cylc-run/foo/share symlink & target already exist, and we're now on share/cycle

The line target.mkdir(...) above will create ~/sym-share/cylc-run/foo/share/cycle.

At this point path exists now - path is ~/cylc-run/foo/share/cycle, but because ~/cylc-run/foo/share is a symlink, path is actually ~/sym-share/cylc-run/foo/share/cycle

(Sorry, the comment in the code doesn't give a good explanation I now realise! And I should have created a test case for share and share/cycle being the same)

cylc/flow/workflow_files.py

tests/unit/conftest.py

cylc/flow/pathutil.py

MetRonnie · 2021-07-01T10:15:32Z

cylc/flow/pathutil.py

-    if path.exists():
-        # Trying to link to itself; no symlink needed
-        # (e.g. path's parent is symlink to target's parent)
-        return False


But this checks if the path exists after the target is created.

When the ~/cylc-run/foo/share -> ~/sym-share/cylc-run/foo/share symlink & target already exist, and we're now on share/cycle

The line target.mkdir(...) above will create ~/sym-share/cylc-run/foo/share/cycle.

At this point path exists now - path is ~/cylc-run/foo/share/cycle, but because ~/cylc-run/foo/share is a symlink, path is actually ~/sym-share/cylc-run/foo/share/cycle

(Sorry, the comment in the code doesn't give a good explanation I now realise! And I should have created a test case for share and share/cycle being the same)

cylc/flow/workflow_files.py

tests/unit/conftest.py

hjoliver

Approving this now, assuming @MetRonnie's final comments will be addressed (or countered). Very nice, thanks @datamel 👍

cylc/flow/workflow_files.py

tests/unit/test_workflow_files.py

symlink-dirs=, cylc play skip local sym-dirs for installed workflows

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com> Rebase Tidy

MetRonnie

Happy with the response to my review

datamel added the rose-suite-run label Jun 8, 2021

datamel added this to the cylc-8.0b2 milestone Jun 8, 2021

datamel self-assigned this Jun 8, 2021

datamel requested a review from oliver-sanders June 9, 2021 09:55

oliver-sanders reviewed Jun 9, 2021

View reviewed changes

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

oliver-sanders reviewed Jun 16, 2021

View reviewed changes

datamel force-pushed the symlink-dirs-cli branch 2 times, most recently from 65a8564 to 8f0883c Compare June 16, 2021 13:52

oliver-sanders reviewed Jun 16, 2021

View reviewed changes

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

oliver-sanders reviewed Jun 16, 2021

View reviewed changes

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

datamel force-pushed the symlink-dirs-cli branch from 69976de to 7ab5c5f Compare June 17, 2021 08:21

datamel mentioned this pull request Jun 17, 2021

symlink dirs cli docs cylc/cylc-doc#252

Merged

oliver-sanders approved these changes Jun 22, 2021

View reviewed changes

oliver-sanders requested a review from hjoliver June 22, 2021 11:07

hjoliver reviewed Jun 23, 2021

View reviewed changes

datamel force-pushed the symlink-dirs-cli branch 3 times, most recently from e3af61f to b6d8a98 Compare June 23, 2021 11:49

datamel mentioned this pull request Jun 29, 2021

cylc clean 3: targeted clean #4237

Merged

7 tasks

datamel force-pushed the symlink-dirs-cli branch 2 times, most recently from cfffcd1 to c98505b Compare June 29, 2021 22:15

MetRonnie reviewed Jun 30, 2021

View reviewed changes

datamel force-pushed the symlink-dirs-cli branch from fde52b9 to 471915a Compare July 1, 2021 09:40

MetRonnie reviewed Jul 1, 2021

View reviewed changes

hjoliver approved these changes Jul 6, 2021

View reviewed changes

datamel force-pushed the symlink-dirs-cli branch from 4027614 to dacbef7 Compare July 6, 2021 16:19

datamel force-pushed the symlink-dirs-cli branch from dacbef7 to b6d32aa Compare July 7, 2021 08:19

MetRonnie reviewed Jul 12, 2021

View reviewed changes

datamel added 9 commits July 20, 2021 13:21

Symlink dirs cli option

e455102

move symlink dirs to install glbl config section

1bdb383

Increase Symlink Dirs Test Coverage

556fd2f

tidy

d627c22

skip symlink creation if any exist

9520546

symlink-dirs=, cylc play skip local sym-dirs for installed workflows

Review Feedback HO

3e45611

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com> Rebase Tidy

review feedback RD

4023cc8

respond to RD's comment request

8424c70

Review feedback RD

c14b904

datamel force-pushed the symlink-dirs-cli branch from 042d1b0 to c14b904 Compare July 20, 2021 12:22

MetRonnie approved these changes Jul 20, 2021

View reviewed changes

oliver-sanders merged commit acf5fdb into cylc:master Jul 21, 2021

datamel deleted the symlink-dirs-cli branch July 21, 2021 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symlink dirs cli #4250

Symlink dirs cli #4250

datamel commented Jun 8, 2021 •

edited

Loading

hjoliver commented Jun 9, 2021

hjoliver commented Jun 9, 2021

oliver-sanders left a comment •

edited

Loading

datamel commented Jun 16, 2021 •

edited

Loading

oliver-sanders commented Jun 17, 2021

hjoliver left a comment

MetRonnie left a comment

MetRonnie Jun 30, 2021

datamel Jul 1, 2021

MetRonnie Jul 1, 2021

MetRonnie Jul 1, 2021

hjoliver left a comment

MetRonnie left a comment

Symlink dirs cli #4250

Symlink dirs cli #4250

Conversation

datamel commented Jun 8, 2021 • edited Loading

hjoliver commented Jun 9, 2021

hjoliver commented Jun 9, 2021

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

datamel commented Jun 16, 2021 • edited Loading

oliver-sanders commented Jun 17, 2021

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie Jun 30, 2021

Choose a reason for hiding this comment

datamel Jul 1, 2021

Choose a reason for hiding this comment

MetRonnie Jul 1, 2021

Choose a reason for hiding this comment

MetRonnie Jul 1, 2021

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

datamel commented Jun 8, 2021 •

edited

Loading

oliver-sanders left a comment •

edited

Loading

datamel commented Jun 16, 2021 •

edited

Loading