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

Unify global and local configs. #3348

Closed
wants to merge 7 commits into from
Closed

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 4, 2019

As part of the rose-suite-run conversion to cylc-??? the config schema for site, local and suite configs should be merged as described by this cylc-admin proposal document.

As a first step I have created a combined schema. This is not yet hooked into any functionality, although it has been checked by changing files that call it and using it to validate a test suite.rc file.

The first commit does nothing except move the file cfgspc/suite.py to config_schema. Changes to the schema are made in the second PR. Reviewers may therefore find it even more than usually useful to review individual commits.

@matthewrmshin matthewrmshin added this to the cylc-8.0a2 milestone Sep 4, 2019
@cylc cylc deleted a comment Sep 5, 2019
@cylc cylc deleted a comment Sep 5, 2019
@cylc cylc deleted a comment Sep 5, 2019
@wxtim wxtim changed the title Create config schema Create config schema (Failing on Codacy - probably ok to ignore?) Sep 5, 2019
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Eyeballed!

cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Show resolved Hide resolved
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
cylc/flow/config_schema.py Show resolved Hide resolved
cylc/flow/config_schema.py Show resolved Hide resolved
cylc/flow/config_schema.py Show resolved Hide resolved
@cylc cylc deleted a comment Sep 13, 2019
@cylc cylc deleted a comment Sep 16, 2019
@cylc cylc deleted a comment Sep 30, 2019
@cylc cylc deleted a comment Sep 30, 2019
@cylc cylc deleted a comment Oct 4, 2019
cylc/flow/config_schema.py Outdated Show resolved Hide resolved
@cylc cylc deleted a comment Oct 7, 2019
@matthewrmshin
Copy link
Contributor

On the positive side, I hope the debate for the configuration schema update is worth the trouble. I now fully believe that the agreed approach is moving the project in the right direction.

@cylc cylc deleted a comment Oct 10, 2019
reorganize upgrade

put upgrader line back in the right place

further changes on OS PR review

responded to many of Matt Shin's Recommendations on config_schema

added necessary import to client_schema

pycodestyle fixes
@cylc cylc deleted a comment Oct 10, 2019
@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #3348 into master will decrease coverage by 0.5%.
The diff coverage is 33.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3348      +/-   ##
==========================================
- Coverage   88.66%   88.16%   -0.51%     
==========================================
  Files         162      164       +2     
  Lines       18000    18172     +172     
  Branches     3914     3945      +31     
==========================================
+ Hits        15960    16021      +61     
- Misses       2040     2151     +111
Impacted Files Coverage Δ
cylc/flow/client_schema.py 100% <100%> (ø)
cylc/flow/config_schema.py 32.94% <32.94%> (ø)
cylc/flow/subprocpool.py 87.93% <0%> (-2.02%) ⬇️
cylc/flow/suite_srv_files_mgr.py 80.66% <0%> (-0.28%) ⬇️
cylc/flow/batch_sys_manager.py 80.78% <0%> (+0.23%) ⬆️
cylc/flow/loggingutil.py 95.79% <0%> (+0.84%) ⬆️
cylc/flow/terminal.py 90.78% <0%> (+7.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6a557f...61676aa. Read the comment docs.

@cylc cylc deleted a comment Oct 11, 2019
@wxtim wxtim changed the title Create config schema Unify global and local configs. Oct 11, 2019
@hjoliver
Copy link
Member

hjoliver commented Oct 14, 2019

Actually @wxtim since this PR is where [cylc] -> [general] I think it would be appropriate to get the section name right here, unless we can't agree quickly. As I commented here: #3404 (comment) these settings are not (or not all, at least) actually "general" in any way. They are just a collection of miscellaneous settings that don't belong under other headings.

As such, I vote for [misc].

None of the synonyms for "miscellaneous" listed here seem more appropriate, much as I like multifarious or promiscuous!

(And we can't use "settings" as suggested somewhere else, because every item in entire the file is a "setting").

 * parameter environment templates --> task parameter environment templates
 * cylc -> general -> misc
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.

👍

@wxtim wxtim closed this Nov 19, 2019
@dpmatthews dpmatthews removed this from the cylc-8.0a2 milestone Nov 19, 2019
@dpmatthews
Copy link
Contributor

We're no longer intending to combine the configs at this stage. We will review all the proposed item renaming as part of #3422.

@datamel datamel mentioned this pull request Nov 29, 2019
2 tasks
@wxtim wxtim deleted the create-config-schema branch May 27, 2020 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants