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

Rsync configurable #4736

Merged
merged 4 commits into from
Mar 22, 2022
Merged

Rsync configurable #4736

merged 4 commits into from
Mar 22, 2022

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Mar 9, 2022 •
edited
Loading

These changes close #4696
The rsync used for remote file installation is now configurable (Note that cylc install rsync is not affected by this change)

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.cfg and conda-environment.yml.
  • Appropriate tests are included (adapted functional test).
  • Appropriate change log entry included.
  • No documentation update required. - Documentation is included by auto documentation of global config.

@datamel datamel self-assigned this Mar 9, 2022
@datamel datamel added this to the cylc-8.0rc3 milestone Mar 9, 2022
@datamel datamel force-pushed the rsync-configurable branch from 1363bbc to 895830e Compare March 9, 2022 13:46
oliver-sanders
oliver-sanders reviewed Mar 9, 2022
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.

Much nicer!

cylc/flow/remote.py Outdated Show resolved Hide resolved
cylc/flow/subprocpool.py Outdated Show resolved Hide resolved
tests/functional/remote/07-slow-file-install/bin/my-rsync.sh Show resolved Hide resolved
@datamel datamel requested a review from wxtim March 17, 2022 12:45
@datamel datamel mentioned this pull request Mar 17, 2022
Closed
wxtim
wxtim reviewed Mar 18, 2022
View reviewed changes
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

I'm not sure that I can see how tests/functional/remote/07-slow-file-install.t works.

I would have expected a call to create_test_global_config which sets rsync command for a platform.

tests/functional/remote/07-slow-file-install/flow.cylc Outdated Show resolved Hide resolved
tests/functional/remote/07-slow-file-install/flow.cylc Outdated Show resolved Hide resolved
tests/functional/remote/07-slow-file-install.t Outdated Show resolved Hide resolved
tests/functional/remote/07-slow-file-install/bin/my-rsync.sh Show resolved Hide resolved
@datamel datamel force-pushed the rsync-configurable branch from 0d97caa to 6f261e2 Compare March 21, 2022 10:11
@wxtim wxtim requested review from wxtim and oliver-sanders March 21, 2022 12:12
wxtim
wxtim approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@wxtim wxtim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • Tested manually as working
  • Checked tests
  • Read code

Style test fails being dealt with at #4746

oliver-sanders
oliver-sanders approved these changes Mar 21, 2022
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.

Tested the test by disabling file installation to make sure it fails correctly.

@oliver-sanders
Copy link
Member

@datamel, all good, could you rebase to pick up the style fixes.

@datamel datamel force-pushed the rsync-configurable branch from 6f261e2 to ba28154 Compare March 22, 2022 11:38
@datamel datamel modified the milestones: cylc-8.0rc3, cylc-8.0rc2 Mar 22, 2022
@datamel datamel merged commit 67178a9 into cylc:master Mar 22, 2022
@datamel datamel deleted the rsync-configurable branch March 22, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@wxtim wxtim wxtim approved these changes

Assignees

@datamel datamel

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

Successfully merging this pull request may close these issues.

tests/f: remote/01 find a better way to simulate slow rsync
3 participants
@datamel @oliver-sanders @wxtim