Global config hierarchy #3802

MetRonnie · 2020-09-02T17:00:52Z

These changes close #3689

Cylc will attempt to load the global configuration (global.cylc) from a hierarchy of locations, including the site directory (defaults to /etc/cylc/flow/) and the user directory at ~/.cylc/flow/. E.g. for Cylc version 8.0.1, the hierarchy would be, in order of ascending priority:

${CYLC_SITE_CONF_PATH}/global.cylc

${CYLC_SITE_CONF_PATH}/8/global.cylc

${CYLC_SITE_CONF_PATH}/8.0/global.cylc

${CYLC_SITE_CONF_PATH}/8.0.1/global.cylc

~/.cylc/flow/global.cylc

~/.cylc/flow/8/global.cylc

~/.cylc/flow/8.0/global.cylc

~/.cylc/flow/8.0.1/global.cylc

A setting in a file lower down in the list will override the same setting from those higher up (but if a setting is present in a file higher up and not in any files lower down, it will not be overridden).

To override the default configuration path, set the CYLC_CONF_PATH environment variable to the directory containing your global.cylc file. If this is set, the hierarchy above is ignored.

Setting the CYLC_SITE_CONF_PATH environment variable overrides the default value of /etc/cylc/flow/, without ignoring the hierarchy.

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).

Appropriate tests are included (unit and/or functional) - kind of 😛

Appropriate change log entry included.

(master branch) I have opened a documentation PR at Configuration file changes cylc-doc#156

No dependency changes.

MetRonnie · 2020-09-02T17:06:16Z

I'm not sure how best to test the global config hierarchy

oliver-sanders · 2020-09-03T09:03:45Z

I'm not sure how best to test the global config hierarchy

Can test the value of SITE_CONF_DIR_HIERARCHY.
If really going for it could move the logic which loads files into it's own function load_config(files: list).

MetRonnie · 2020-09-03T10:45:08Z

cylc/flow/cfgspec/globalcfg.py

            if os.access(fname, os.F_OK | os.R_OK):
                self.loadcfg(fname, upgrader.USER_CONFIG)
+            else:
+                LOG.warning(
+                    f'No global.cylc file at CYLC_CONF_PATH: {conf_path_str}')


Is this useful? I noticed there was a previous commit to raise an error if the path doesn't exist, but that commit was reverted

Because this environment variable can get copied to remote platforms (which aren't on the same filesystem) this warning is useful some of the time, and misleading some of the time.

Ok, removing it on the basis that being misleading some of the time is worse than not being helpful some of the time

MetRonnie · 2020-09-03T17:51:31Z

I've found it messy trying to unit test the exact config dir hierarchy, but at least there is a doctest for the version number hierarchy:

cylc-flow/cylc/flow/cfgspec/globalcfg.py

Lines 698 to 707 in 564bf83

    
           def get_version_hierarchy(version): 
        
               """Return list of versions whose global configs are compatible, in 
        
               ascending priority. 
        
               Args: 
        
                   version (str): A PEP 440 compliant version number. 
        
               Example: 
        
                   >>> get_version_hierarchy('8.0.1a2.dev') 
        
                   ['', '8', '8.0', '8.0.1', '8.0.1a2', '8.0.1a2.dev']

and a unit test for the override behaviour of ParsecConfig.loadcfg

cylc-flow/tests/unit/parsec/test_config.py

Lines 75 to 77 in 564bf83

    
           def test_loadcfg_override(sample_spec): 
        
               """Test that loading a second config file overrides common settings but 
        
               leaves in settings only present in the first"""

(There is no single site/user global config dir with the new hierarchy) Also remove site global-tests.cylc functionality, only use user global-tests.cylc

Plus fix a pycodestyle "error"

oliver-sanders · 2020-09-10T16:48:07Z

@MetRonnie a rebase should fix test failure

MetRonnie · 2020-09-10T17:58:50Z

Just undoing & redoing the last commit and force pushing seems to have done the trick, as the tests run on a merge commit (PR branch merged into master). Ideally the tests should re-run when master changes, then. But I don't think that's possible currently

oliver-sanders · 2020-09-11T08:30:42Z

That's interesting, didn't know it worked like that, Travis CI just checked out your branch locally so you had to rebase to pick up changes which resulted in a couple of false negatives over the years.

MetRonnie · 2020-09-11T09:09:18Z

Travis also creates a merge commit to run the tests, e.g. for https://travis-ci.org/github/metomi/isodatetime/builds/725975670 it says " Commit ba1e4c1 " and if you click on the link it shows

oliver-sanders

Looks good, one style comment.

oliver-sanders · 2020-09-11T11:27:53Z

cylc/flow/cfgspec/globalcfg.py

+        self.SITE_CONF_PATH = (os.getenv('CYLC_SITE_CONF_PATH') or
+                               os.path.join(os.sep, 'etc', 'cylc', 'flow'))
+        self.USER_CONF_PATH = os.path.join(self._HOME, '.cylc', 'flow')
+        version_hierarchy = get_version_hierarchy(CYLC_VERSION)
+        self.CONF_DIR_HIERARCHY = [
+            *[(upgrader.SITE_CONFIG, os.path.join(self.SITE_CONF_PATH, ver))
+              for ver in version_hierarchy],
+            *[(upgrader.USER_CONFIG, os.path.join(self.USER_CONF_PATH, ver))
+              for ver in version_hierarchy]
+        ]


If converting these class attributes to instance attributes make them lower case.

It was only CONF_DIR_HIERARCHY that needed to be moved inside __init__ because it's not possible to use another class attribute inside the list comprehension (https://stackoverflow.com/a/13913933/3217306). Seeing as this is the only reason for making it a instance attribute, would it make sense to leave it uppercase if the other two are changed to class attributes?

hjoliver

LGTM 👍 thanks @MetRonnie

MetRonnie added this to the cylc-8.0a3 milestone Sep 2, 2020

MetRonnie requested review from hjoliver and oliver-sanders September 2, 2020 17:00

MetRonnie self-assigned this Sep 2, 2020

MetRonnie mentioned this pull request Sep 2, 2020

Configuration file changes cylc/cylc-doc#156

Merged

MetRonnie commented Sep 3, 2020

View reviewed changes

MetRonnie force-pushed the config-changes2 branch from 6a64ada to 06c16c2 Compare September 3, 2020 17:22

MetRonnie added 7 commits September 4, 2020 14:47

Implement global config hierarchy

0f6c567

Remove --print-site-dir/--print-user-dir from cylc get-config

5978e35

(There is no single site/user global config dir with the new hierarchy) Also remove site global-tests.cylc functionality, only use user global-tests.cylc

Implement usage of CYLC_SITE_CONF_PATH env var

3fb08a7

Fix tests & tidy

527f102

Update changelog

530d201

Global config hierarchy: Separate dev from prerelease

1f90b8b

Plus fix a pycodestyle "error"

Tidy/slight refactor

d8307e9

MetRonnie force-pushed the config-changes2 branch from 564bf83 to 7537026 Compare September 4, 2020 13:49

Add unit test for parsec loadcfg's override behaviour

c3b030e

MetRonnie force-pushed the config-changes2 branch from 7537026 to c3b030e Compare September 10, 2020 17:42

oliver-sanders approved these changes Sep 11, 2020

View reviewed changes

hjoliver approved these changes Sep 17, 2020

View reviewed changes

hjoliver merged commit 8406f4e into cylc:master Sep 17, 2020

MetRonnie deleted the config-changes2 branch September 17, 2020 09:01

hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global config hierarchy #3802

Global config hierarchy #3802

MetRonnie commented Sep 2, 2020 •

edited

Loading

MetRonnie commented Sep 2, 2020

oliver-sanders commented Sep 3, 2020

MetRonnie Sep 3, 2020

oliver-sanders Sep 3, 2020

MetRonnie Sep 3, 2020

MetRonnie commented Sep 3, 2020

oliver-sanders commented Sep 10, 2020

MetRonnie commented Sep 10, 2020

oliver-sanders commented Sep 11, 2020

MetRonnie commented Sep 11, 2020

oliver-sanders left a comment •

edited

Loading

oliver-sanders Sep 11, 2020

MetRonnie Sep 14, 2020

hjoliver left a comment

Global config hierarchy #3802

Global config hierarchy #3802

Conversation

MetRonnie commented Sep 2, 2020 • edited Loading

MetRonnie commented Sep 2, 2020

oliver-sanders commented Sep 3, 2020

MetRonnie Sep 3, 2020

Choose a reason for hiding this comment

oliver-sanders Sep 3, 2020

Choose a reason for hiding this comment

MetRonnie Sep 3, 2020

Choose a reason for hiding this comment

MetRonnie commented Sep 3, 2020

oliver-sanders commented Sep 10, 2020

MetRonnie commented Sep 10, 2020

oliver-sanders commented Sep 11, 2020

MetRonnie commented Sep 11, 2020

oliver-sanders left a comment • edited Loading

Choose a reason for hiding this comment

oliver-sanders Sep 11, 2020

Choose a reason for hiding this comment

MetRonnie Sep 14, 2020

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

MetRonnie commented Sep 2, 2020 •

edited

Loading

oliver-sanders left a comment •

edited

Loading