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.

Sign up for GitHub

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

Jump to bottom

Cylc install update #4193

Merged
merged 6 commits into from
May 18, 2021
Merged

Cylc install update #4193

merged 6 commits into from
May 18, 2021

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Apr 23, 2021 •
edited
Loading

This PR addresses some feedback on cylc install from @dpmatthews.

  • Logging of symlink making was inaccurate. (added a condition to dict used for logging)
  • Tidied the symlinks created for suite.rc and runN so they are now relative paths
  • Symlink dirs env variables had been changed to being expanded on the scheduler, (reverted that change)
  • Standard cylc install now correctly installs from directories with a . in the name
  • Updated the cylc install help to correctly document location of source symlink.
  • Localhost symlinks are now skipped if they exist but point to another location, rather that raising a Workflow Files Error
  • cylc install now logs the named run rather than flow_name

This is a small change with no associated Issue.

  • 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).
  • Already covered by existing tests (adapted).
  • Appropriate change log entry included.
  • No documentation update required.
  • No dependency changes.

@datamel datamel added this to the cylc-8.0b2 milestone Apr 23, 2021
@datamel datamel self-assigned this Apr 23, 2021
MetRonnie
MetRonnie reviewed Apr 23, 2021
View reviewed changes
tests/unit/test_pathutil.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
tests/unit/test_pathutil.py Show resolved Hide resolved
@oliver-sanders
Copy link
Member

Happy conflict day!

@datamel datamel force-pushed the cylc-install-update branch 3 times, most recently from 8b63f73 to fa248cd Compare April 28, 2021 16:15
MetRonnie
MetRonnie reviewed Apr 28, 2021
View reviewed changes
cylc/flow/pathutil.py Show resolved Hide resolved
cylc/flow/workflow_files.py Show resolved Hide resolved
@datamel
Ensure symlink dirs vars are unexpanded on scheduler before remote in…
8938748 
…it. Display named run in cylc install output
@datamel
Tidy symlink to relative path.
668776e 
Enable directories containing a '.' to be installed with the correct name.
@datamel
Update tests
71a3f1c 
Respond to review feedback from RD
@datamel
Respond to feedback from RD
5caf6f1
oliver-sanders
oliver-sanders approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Code looks good, my tests ran fine.

@oliver-sanders oliver-sanders merged commit fd31f7e into cylc:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpmatthews dpmatthews dpmatthews left review comments

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@datamel datamel

Labels
None yet
Projects
None yet
Milestone
cylc-8.0b2
Development

Successfully merging this pull request may close these issues.
4 participants
@datamel @oliver-sanders @dpmatthews @MetRonnie