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.

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

conf: change suite.rc to flow.cylc #3755

Merged
merged 27 commits into from
Aug 18, 2020
Merged

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Aug 5, 2020

These changes partially address #3689

Summary

  • suite.rc --> flow.cylc
  • flow.rc & global.rc --> global.cylc
  • The directory log/suiterc/ --> log/flow-config/
  • Generally, suiterc in variable names --> flow_config, and generally rc in variable names --> config
    • Functional tests: test_header function create_test_globalrc --> create_test_global_config
    • Integration tests: (in tests/i/utils/flow_writer.py): flow_writer.suiterc() --> flow_writer.flow_config_str()
    • When referring to a file or filename/path (rather than the config itself), generally suiterc --> flow_file
  • Every other *.rc --> *.cylc

Backwards compatibility

  • The first time a user cylc runs a directory that contains only suite.rc, it will automatically create a symlink flow.cylc to that suite.rc.
  • If a user cylc runs an already-registered suite/workflow and flow.cylc doesn't exist (i.e. suite/workflow was run on a previous version of Cylc), it will re-register

(No backwards compatibility for flow.rc)


Best to look at this one commit by commit. The commits called "Wider rename..." usually consist of the same change done across many files, so you probably only need to take a look at a couple of the files in those.

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).
  • Tests for "upgrading" existing suite.rc to flow.cylc included
  • Appropriate change log entry included.
  • No dependency changes.

@MetRonnie MetRonnie added this to the cylc-8.0a3 milestone Aug 5, 2020
@MetRonnie MetRonnie self-assigned this Aug 5, 2020
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
@MetRonnie MetRonnie mentioned this pull request Aug 5, 2020
5 tasks
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
@MetRonnie MetRonnie force-pushed the config-changes branch 2 times, most recently from 3fb6336 to 1e718cb Compare August 6, 2020 16:39
@MetRonnie MetRonnie changed the title Configuration file changes conf: change suite.rc to flow.cylc Aug 6, 2020
If a workflow dir only has 'suite.rc' on first run, automatically create
a symlink 'flow.cylc' to that 'suite.rc' in the register suite step
@@ -87,21 +87,21 @@

Copy link
Member

Choose a reason for hiding this comment

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

You will need to rename the configuration file at line 37.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean flow.rc? Would it make sense to split that off into another PR?

Copy link
Member

@oliver-sanders oliver-sanders Aug 7, 2020

Choose a reason for hiding this comment

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

Ah, just read the PR description, I would have thought it would be easier to do both at the same time, but if you want to do it in two PRs that's ok.

cylc/flow/etc/syntax/cylc.vim Outdated Show resolved Hide resolved
cylc/flow/etc/syntax/cylc.lang Outdated Show resolved Hide resolved
cylc/flow/etc/syntax/cylc-mode.el Outdated Show resolved Hide resolved
cylc/flow/etc/syntax/cylc.xml Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member

oliver-sanders commented Aug 7, 2020

If a user cylc runs an already-registered suite/workflow and flow.cylc doesn't exist (i.e. suite/workflow was run on a previous version of Cylc), it will re-register

is this overkill

No, it's a really valuable feature for those using Cylc without Rose.

For those using Cylc with Rose it's a potential trap, but that's something we can address with the rose suite-run work later on.

@hjoliver
Copy link
Member

The directory log/suiterc/ --> log/flow_config/

A very minor point, but the cylc-run directory uses - rather than _, so for consistency I'd prefer flow-config here.

Actually maybe just config or conf would do - we're not going to be storing other config info here are we?

@MetRonnie
Copy link
Member Author

maybe just config or conf would do - we're not going to be storing other config info here are we?

As a relative newcomer, I feel that including "flow" in the name makes its purpose clearer.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM 👍

cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved
cylc/flow/cfgspec/suite.py Outdated Show resolved Hide resolved
cylc/flow/parsec/README.md Show resolved Hide resolved
cylc/flow/scripts/cylc_graph.py Show resolved Hide resolved
@MetRonnie MetRonnie marked this pull request as ready for review August 13, 2020 12:26
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

(Re-approved after a quick scroll through "Files changed")

for cases such as `cylc validate .` where the dir only contains deprecated suite.rc
@MetRonnie
Copy link
Member Author

MetRonnie commented Aug 13, 2020

Just pushed one last change because commands like cylc validate . weren't working for suites like ~/cylc-run/foo/bar/ (i.e. the suite name is a bunch of nested directories) with only suite.rc present

- Remove previously conflicting files that were deleted on master
- Put trailing spaces back in whitespace test
- (Also fix call to missing function in cylc-scan sigstop test)
@oliver-sanders
Copy link
Member

I think we are good to go!

@oliver-sanders oliver-sanders merged commit abd4728 into cylc:master Aug 18, 2020
@MetRonnie MetRonnie deleted the config-changes branch August 18, 2020 11:32
@MetRonnie MetRonnie mentioned this pull request Aug 24, 2020
7 tasks
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conf: configuration file changes
3 participants