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 Reinstall #4071

Merged
merged 8 commits into from
Feb 19, 2021
Merged

Cylc Reinstall #4071

merged 8 commits into from
Feb 19, 2021

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Feb 10, 2021 •
edited
Loading

Cylc Reinstall Command

Follows on from Cylc Install (#4000)
Users can now reinstall an already installed workflow using the cylc reinstall command.

Proposal: https://github.com/cylc/cylc-admin/blob/master/docs/proposal-rose-suite-run.md

Example use cases:

cylc install myflow
cylc reinstall myflow/run1

cylc install myflow --no-run-name
cylc reinstall myflow

cylc reinstall from within an installed flow in the cylc run dir.

New reinstall log added in log directory: log/install/timestamp-reinstall.log

I have also addressed an issue (related change, #4000 (review)) with the symlink of flow.cylc/suite.rc file being created in the source directory. It is now created in the run dir.

These changes close #3890

  • 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).
  • Appropriate tests are included (unit and/or functional).

@datamel datamel self-assigned this Feb 10, 2021
@datamel datamel added this to the cylc-8.0b0 milestone Feb 10, 2021
@datamel
Copy link
Contributor Author

datamel commented Feb 11, 2021

@wxtim I'd value your opinion on this, particularly the cylc-rose side of the reinstall. Flagging you as a reviewer for this, if that is ok?

wxtim
wxtim reviewed Feb 11, 2021
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.

Adding a comment-review checklist, so that I can communicate with you what I am doing with your branch.

  • Read. Checked for obvious code and spelling, punctuation & grammar issues.
  • Run tests locally.
  • Run tests on cylc-rose.
  • Check that tests are OK with and without cylc-rose installed. One of the tests failed without Cylc-Rose. I have created a PR to your PR branch as a suggestion.
  • Try it on some home-made examples.
  • Double check against proposal.

@wxtim wxtim self-requested a review February 11, 2021 13:35
wxtim
wxtim reviewed Feb 11, 2021
View reviewed changes
tests/functional/cylc-reinstall/00-simple.t Outdated
popd || exit 1
purge_rnd_suite

# Test basic cylc reinstall, named run (including flow name) given
Copy link
Member

Choose a reason for hiding this comment

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

petty:

Suggested change
# Test basic cylc reinstall, named run (including flow name) given
# Test basic cylc reinstall, named run (including ``flow.cylc`` name) given.

Copy link
Member

@oliver-sanders oliver-sanders Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(reply to tim)

I think it is a flow name (i.e. reg) rather than a flow.cylc path?

Copy link
Member

Choose a reason for hiding this comment

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

Line 43?

@wxtim wxtim self-requested a review February 11, 2021 14:34
oliver-sanders
oliver-sanders reviewed Feb 11, 2021
View reviewed changes
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/scripts/reinstall.py Outdated Show resolved Hide resolved
tests/functional/cylc-reinstall/00-simple.t Outdated
popd || exit 1
purge_rnd_suite

# Test basic cylc reinstall, named run (including flow name) given
Copy link
Member

@oliver-sanders oliver-sanders Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

(reply to tim)

I think it is a flow name (i.e. reg) rather than a flow.cylc path?

@oliver-sanders
Copy link
Member

BTW: The rose integration will require a little extra work (some CLI opts to add, etc) but that's not for this PR.

@wxtim wxtim self-requested a review February 11, 2021 15:52
wxtim
wxtim approved these changes Feb 11, 2021
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.

  • Read. Checked for obvious code and spelling, punctuation & grammar issues.

  • Run tests locally.

  • Run tests on cylc-rose.
    Approved, subject to dealing with comments from @oliver-sanders

  • Check that tests are OK with and without cylc-rose installed. One of the tests failed without Cylc-Rose. I have created a PR to your PR branch as a suggestion.

  • Try it on some home-made examples.

  • Double check against proposal.

@datamel datamel force-pushed the cylc-reinstall branch 2 times, most recently from dc2f000 to 11d9a31 Compare February 17, 2021 09:35
datamel and others added 7 commits February 18, 2021 09:49
@datamel
Add cylc reinstall
a35d569
@datamel
cylc reinstall
c6fd91b 
Add functional tests for cylc reinstall
@datamel
Add tests for cylc reinstall
79bb2d6
@datamel
Add functional test for cylc reinstall
d174245
@datamel
Convert functional tests to unit
c957a11
@datamel @oliver-sanders
Update cylc/flow/scripts/reinstall.py
c83f202 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@wxtim @datamel
Fixed over sensitivity of test:
82bec84 
tests/functional/cylc-reinstall/01-file-transfer-basic-tree.t to
presence/absence of cylc-rose.
oliver-sanders
oliver-sanders reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders 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.

Some suggested error message changes (untested):

cylc/flow/scripts/reinstall.py Outdated Show resolved Hide resolved
cylc/flow/scripts/reinstall.py Outdated Show resolved Hide resolved
cylc/flow/scripts/reinstall.py Outdated Show resolved Hide resolved
cylc/flow/scripts/reinstall.py Outdated Show resolved Hide resolved
@datamel
Copy link
Contributor Author

datamel commented Feb 18, 2021

Some suggested error message changes (untested):

Thanks, I'll just fix the tests.

@oliver-sanders
Copy link
Member

Just some style issues with line length (sorry), do squash down those last commits when you push back. All good here thanks.

@datamel datamel force-pushed the cylc-reinstall branch 2 times, most recently from c755a00 to 8320cac Compare February 18, 2021 17:06
@datamel
Reduce test number cylc-reinstall
510074e 
Make error message more informative

Update cylc/flow/scripts/reinstall.py

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

Update error messages in tests

Update pytest test_suite_files.py
@oliver-sanders oliver-sanders merged commit 45260a4 into cylc:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxtim wxtim wxtim approved these changes

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

Assignees

@datamel datamel

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

Successfully merging this pull request may close these issues.

cylc reinstall
3 participants
@datamel @oliver-sanders @wxtim