diff --git a/CHANGES.md b/CHANGES.md index 54b4f11610e..54d15ab2ab7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -21,6 +21,10 @@ configuration files. ### Fixes +[5450](https://github.com/cylc/cylc-flow/pull/5450) - Validation provides +better error messages if [sections] and settings are mixed up in a +configuration. + [5445](https://github.com/cylc/cylc-flow/pull/5445) - Fix remote tidy bug where install target is not explicit in platform definition. diff --git a/cylc/flow/config.py b/cylc/flow/config.py index a549c9ed771..e5a6e7a1e54 100644 --- a/cylc/flow/config.py +++ b/cylc/flow/config.py @@ -356,13 +356,6 @@ def __init__( except KeyError: parameter_templates = {} - # Check that parameter templates are a section - if not hasattr(parameter_templates, 'update'): - raise WorkflowConfigError( - '[task parameters][templates] is a section. Don\'t use it ' - 'as a parameter.' - ) - # parameter values and templates are normally needed together. self.parameters = (parameter_values, parameter_templates) diff --git a/cylc/flow/parsec/upgrade.py b/cylc/flow/parsec/upgrade.py index 0c15b4ff1a3..e2e9840bca6 100644 --- a/cylc/flow/parsec/upgrade.py +++ b/cylc/flow/parsec/upgrade.py @@ -104,7 +104,13 @@ def obsolete(self, vn, oldkeys, silent=False, is_section=False): def get_item(self, keys): item = self.cfg for key in keys: - item = item[key] + try: + item = item[key] + except TypeError: + raise UpgradeError( + f'{self.show_keys(keys[:-1], True)}' + f' ("{keys[-2]}" should be a [section] not a setting)' + ) return item def put_item(self, keys, val): diff --git a/cylc/flow/parsec/validate.py b/cylc/flow/parsec/validate.py index 70bde7678bb..f6e40e6a912 100644 --- a/cylc/flow/parsec/validate.py +++ b/cylc/flow/parsec/validate.py @@ -204,10 +204,28 @@ def validate(self, cfg_root, spec_root): else: speckey = key specval = spec[speckey] - if isinstance(value, dict) and not specval.is_leaf(): + + cfg_is_section = isinstance(value, dict) + spec_is_section = not specval.is_leaf() + if cfg_is_section and not spec_is_section: + # config is a [section] but it should be a setting= + raise IllegalItemError( + keys, + key, + msg=f'"{key}" should be a setting not a [section]', + ) + if (not cfg_is_section) and spec_is_section: + # config is a setting= but it should be a [section] + raise IllegalItemError( + keys, + key, + msg=f'"{key}" should be a [section] not a setting', + ) + + if cfg_is_section and spec_is_section: # Item is dict, push to queue queue.append([value, specval, keys + [key]]) - elif value is not None and specval.is_leaf(): + elif value is not None and not spec_is_section: # Item is value, coerce according to value type cfg[key] = self.coercers[specval.vdr](value, keys + [key]) if specval.options: diff --git a/tests/functional/validate/76-section-section-transpose.t b/tests/functional/validate/76-section-section-transpose.t new file mode 100644 index 00000000000..6ca90ff0c2e --- /dev/null +++ b/tests/functional/validate/76-section-section-transpose.t @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +#------------------------------------------------------------------------------- +# Test handling of mixed up sections vs settings + +. "$(dirname "$0")/test_header" + +set_test_number 6 + +# 1. section as setting (normal) +TEST_NAME='section-as-setting-normal' +cat > 'flow.cylc' <<__HEREDOC__ +[runtime] + [[foo]] + environment = 42 +__HEREDOC__ +run_fail "${TEST_NAME}-validate" cylc validate . +grep_ok \ + 'IllegalItemError: \[runtime\]\[foo\]environment - ("environment" should be a \[section\] not a setting)' \ + "${TEST_NAME}-validate.stderr" + + +# 2. section as setting (via upgrader) +# NOTE: if this test fails it is likely because the upgrader for "scheduling" +# has been removed, convert this to use a new deprecated section +TEST_NAME='section-as-setting-upgrader' +cat > 'flow.cylc' <<__HEREDOC__ +scheduling = 22 +__HEREDOC__ + +run_fail "${TEST_NAME}-validate" cylc validate . +grep_ok \ + 'UpgradeError: \[scheduling\] ("scheduling" should be a \[section\] not a setting' \ + "${TEST_NAME}-validate.stderr" + + +# 3. setting as section +TEST_NAME='setting-as-section' +cat > 'flow.cylc' <<__HEREDOC__ +[scheduling] + [[initial cycle point]] +__HEREDOC__ + +run_fail "${TEST_NAME}-validate" cylc validate . +grep_ok \ + 'IllegalItemError: \[scheduling\]initial cycle point - ("initial cycle point" should be a setting not a \[section\])' \ + "${TEST_NAME}-validate.stderr" diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index bc256f7cb60..b3799f213da 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -172,22 +172,6 @@ def test_no_graph(flow, validate): assert 'missing [scheduling][[graph]] section.' in str(exc_ctx.value) -def test_parameter_templates_setting(flow, one_conf, validate): - """It should fail if [task parameter]templates is a setting. - - It should be a section. - """ - reg = flow({ - **one_conf, - 'task parameters': { - 'templates': 'foo' - } - }) - with pytest.raises(WorkflowConfigError) as exc_ctx: - validate(reg) - assert '[templates] is a section' in str(exc_ctx.value) - - @pytest.mark.parametrize( 'section', [ 'external-trigger',