From 2e69179add8b740674bd5162ac2e44aeb516981e Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 10:46:38 +0200 Subject: [PATCH 1/9] Moved input data validation from commands.py to validate.py The _compare_dict() / _compare_list() helpers in set_input_command() are moved to validate.py as input_data_matches_spec(), so the same check can be reused by the render-input command which will later be implemented in ticket ENT-11346. Signed-off-by: Lars Erik Wik --- cfbs/commands.py | 48 ++------- cfbs/validate.py | 58 +++++++++++ tests/test_validate.py | 229 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 292 insertions(+), 43 deletions(-) diff --git a/cfbs/commands.py b/cfbs/commands.py index ce92368b..8ca8e83c 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -98,6 +98,7 @@ def search_command(terms: List[str]): ) from cfbs.cfbs_config import CFBSConfig, CFBSReturnWithoutCommit from cfbs.validate import ( + input_data_matches_spec, validate_config, validate_config_raise_exceptions, validate_module_name_content, @@ -1603,48 +1604,11 @@ def set_input_command(name, infile): return CFBSCommandGitResult(1) log.debug("Input data for module '%s': %s" % (name, pretty(data))) - def _compare_dict(a, b, ignore=None): - assert isinstance(a, dict) and isinstance(b, dict) - ignore = ignore or set() - if set(a.keys()) != set(b.keys()) - ignore: - return False - # Avoid code duplication by converting the values of the two dicts - # into two lists in the same order and compare the lists instead - keys = a.keys() - return _compare_list([a[key] for key in keys], [b[key] for key in keys]) - - def _compare_list(a, b): - assert isinstance(a, list) and isinstance(b, list) - if len(a) != len(b): - return False - for x, y in zip(a, b): - if type(x) is not type(y): - return False - if isinstance(x, dict): - if not _compare_dict(x, y): - return False - elif isinstance(x, list): - if not _compare_list(x, y): - return False - else: - assert x is None or isinstance( - x, (int, float, str, bool) - ), "Illegal value type" - if x != y: - return False - return True - - for a, b in zip(spec, data): - if ( - not isinstance(a, dict) - or not isinstance(b, dict) - or not _compare_dict(a, b, ignore=set({"response"})) - ): - log.error( - "Input data for module '%s' does not conform with input definition" - % name - ) - return CFBSCommandGitResult(1) + if not input_data_matches_spec(spec, data): + log.error( + "Input data for module '%s' does not conform with input definition" % name + ) + return CFBSCommandGitResult(1) path = os.path.join(name, "input.json") diff --git a/cfbs/validate.py b/cfbs/validate.py index 236935a5..ca1e3662 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -767,6 +767,64 @@ def _validate_module_input(name, module): ) +def _compare_dict(a, b, ignore=None): + assert isinstance(a, dict) and isinstance(b, dict) + ignore = ignore or set() + if set(a.keys()) != set(b.keys()) - ignore: + return False + # Avoid code duplication by converting the values of the two dicts + # into two lists in the same order and compare the lists instead + keys = a.keys() + return _compare_list([a[key] for key in keys], [b[key] for key in keys]) + + +def _compare_list(a, b): + assert isinstance(a, list) and isinstance(b, list) + if len(a) != len(b): + return False + for x, y in zip(a, b): + if type(x) is not type(y): + return False + if isinstance(x, dict): + if not _compare_dict(x, y): + return False + elif isinstance(x, list): + if not _compare_list(x, y): + return False + else: + assert x is None or isinstance( + x, (int, float, str, bool) + ), "Illegal value type" + if x != y: + return False + return True + + +def input_data_matches_spec(spec, data): + """Check that input data conforms with a module's input definition. + + Compares each element of the input definition (the module's "input") with + the corresponding element of the input data, ignoring the "response" key, + which is what the user adds. + + Note that this checks the input _data_ (an input definition along with the + user's responses), not the input _definition_ in cfbs.json, which is checked + by _validate_module_input() above. + + :param spec: the module's input definition + :param data: the input data to check + :return: True if the input data conforms with the input definition + """ + for a, b in zip(spec, data): + if ( + not isinstance(a, dict) + or not isinstance(b, dict) + or not _compare_dict(a, b, ignore=set({"response"})) + ): + return False + return True + + def validate_single_module(context, name, module, config, local_check=False): """Function to validate one module object. diff --git a/tests/test_validate.py b/tests/test_validate.py index 5b21681e..53962daa 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -1,7 +1,7 @@ import pytest from cfbs.utils import CFBSValidationError -from cfbs.validate import validate_module_name_content +from cfbs.validate import input_data_matches_spec, validate_module_name_content def test_validate_module_name_content(): @@ -30,3 +30,230 @@ def test_validate_module_name_content(): validate_module_name_content("./bad-extension.zip") validate_module_name_content("./123 Illeg@l!/legal-name.cf") + + +def test_input_data_matches_spec_string(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + data = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + # The response is what the user adds, it is ignored when comparing: + assert input_data_matches_spec(spec, data) + + # Input data without a response conforms as well: + assert input_data_matches_spec(spec, spec) + + +def test_input_data_matches_spec_list(): + spec = [ + { + "type": "list", + "variable": "files", + "label": "Files", + "subtype": [ + { + "key": "name", + "type": "string", + "label": "Name", + "question": "What file should this module create?", + }, + { + "key": "content", + "type": "string", + "label": "Content", + "question": "What content should this file have?", + }, + ], + "while": "Do you want to create another file?", + } + ] + data = [ + { + "type": "list", + "variable": "files", + "label": "Files", + "subtype": [ + { + "key": "name", + "type": "string", + "label": "Name", + "question": "What file should this module create?", + }, + { + "key": "content", + "type": "string", + "label": "Content", + "question": "What content should this file have?", + }, + ], + "while": "Do you want to create another file?", + "response": [ + {"name": "/tmp/one.txt", "content": "Hello CFEngine!"}, + {"name": "/tmp/two.txt", "content": "Bye CFEngine!"}, + ], + } + ] + assert input_data_matches_spec(spec, data) + + # A subtype which doesn't match the input definition does not conform: + data[0]["subtype"][1]["key"] = "bogus" + assert not input_data_matches_spec(spec, data) + + +def test_input_data_matches_spec_multiple_variables(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + }, + { + "type": "string", + "variable": "content", + "label": "Content", + "question": "What content should this file have?", + }, + ] + data = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + }, + { + "type": "string", + "variable": "content", + "label": "Content", + "question": "What content should this file have?", + "response": "Hello CFEngine!", + }, + ] + assert input_data_matches_spec(spec, data) + + data[1]["variable"] = "bogus" + assert not input_data_matches_spec(spec, data) + + +def test_input_data_matches_spec_reordered_keys(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + # The order of the keys doesn't matter: + data = [ + { + "variable": "filename", + "type": "string", + "label": "Filename", + "response": "/tmp/create-single-file.txt", + "question": "What file should this module create?", + } + ] + assert input_data_matches_spec(spec, data) + + +def test_input_data_matches_spec_changed_value(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + data = [ + { + "type": "string", + "variable": "bogus", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + assert not input_data_matches_spec(spec, data) + + +def test_input_data_matches_spec_renamed_key(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + data = [ + { + "doofus": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + assert not input_data_matches_spec(spec, data) + + +def test_input_data_matches_spec_missing_or_extra_key(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + missing = [ + { + "type": "string", + "variable": "filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + assert not input_data_matches_spec(spec, missing) + + extra = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "extra": "not in the input definition", + "response": "/tmp/create-single-file.txt", + } + ] + assert not input_data_matches_spec(spec, extra) + + +def test_input_data_matches_spec_not_objects(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + assert not input_data_matches_spec(spec, ["not an object"]) + assert not input_data_matches_spec(["not an object"], spec) From d0bb86e05c6fb0569ef6e754c933ea40b1e57b9f Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 10:56:55 +0200 Subject: [PATCH 2/9] Moved augment generation from build.py to augments.py Moved _generate_augment() to cfbs/augments.py so that it can be reused by the render-input command which will later be implemented in ticket ENT-11346. Signed-off-by: Lars Erik Wik --- cfbs/augments.py | 38 ++++++++ cfbs/build.py | 37 +------- tests/test_augments.py | 199 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 239 insertions(+), 35 deletions(-) create mode 100644 cfbs/augments.py create mode 100644 tests/test_augments.py diff --git a/cfbs/augments.py b/cfbs/augments.py new file mode 100644 index 00000000..60d87285 --- /dev/null +++ b/cfbs/augments.py @@ -0,0 +1,38 @@ +""" +Functions for generating CFEngine augments (def.json) +""" + +from cfbs.utils import canonify + + +def generate_augment(module_name, input_data): + """ + Generate augment from input data. + + :param module_name: name of module + :param input_data: input data + :return: generated augment or None if input data is incomplete + """ + if not isinstance(input_data, list): + return None + + augment = {"variables": {}} + + for variable in input_data: + if not isinstance(variable, dict) or any( + key not in variable for key in ("variable", "response") + ): + continue + + name = variable["variable"] + namespace = variable.get("namespace", "cfbs") + bundle = variable.get("bundle", canonify(module_name)) + value = variable["response"] + comment = variable.get("comment", "Added by 'cfbs input'") + + augment["variables"]["%s:%s.%s" % (namespace, bundle, name)] = { + "value": value, + "comment": comment, + } + + return augment diff --git a/cfbs/build.py b/cfbs/build.py index 7c295fbd..079a0fe0 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -15,10 +15,10 @@ import logging as log import shutil import subprocess +from cfbs.augments import generate_augment from cfbs.cfbs_config import CFBSConfig from cfbs.utils import ( CFBSUserError, - canonify, cli_tool_present, cp, cp_dry_overwrites, @@ -54,39 +54,6 @@ def init_out_folder(): mkdir("out/steps") -def _generate_augment(module_name, input_data): - """ - Generate augment from input data. - - :param module_name: name of module - :param input_data: input data - :return: generated augment or None if input data is incomplete - """ - if not isinstance(input_data, list): - return None - - augment = {"variables": {}} - - for variable in input_data: - if not isinstance(variable, dict) or any( - key not in variable for key in ("variable", "response") - ): - continue - - name = variable["variable"] - namespace = variable.get("namespace", "cfbs") - bundle = variable.get("bundle", canonify(module_name)) - value = variable["response"] - comment = variable.get("comment", "Added by 'cfbs input'") - - augment["variables"]["%s:%s.%s" % (namespace, bundle, name)] = { - "value": value, - "comment": comment, - } - - return augment - - def _perform_replacement(n, a, b, filename): assert n and a and b and filename assert a not in b @@ -297,7 +264,7 @@ def _perform_input_step(args, name, destination, prefix): ) return extras, original = read_json(src), read_json(dst) - extras = _generate_augment(name, extras) + extras = generate_augment(name, extras) log.debug("Generated augment: %s", pretty(extras)) if not extras: raise CFBSExitError( diff --git a/tests/test_augments.py b/tests/test_augments.py new file mode 100644 index 00000000..59c8b044 --- /dev/null +++ b/tests/test_augments.py @@ -0,0 +1,199 @@ +from cfbs.augments import generate_augment + + +def test_generate_augment_string(): + """The "create single file" example from JSON.md""" + input_data = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + assert generate_augment("create-single-file", input_data) == { + "variables": { + "cfbs:create_single_file.filename": { + "value": "/tmp/create-single-file.txt", + "comment": "Added by 'cfbs input'", + } + } + } + + +def test_generate_augment_multiple_variables(): + """The "create a single file with content" example from JSON.md""" + input_data = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + }, + { + "type": "string", + "variable": "content", + "label": "Content", + "question": "What content should this file have?", + "response": "Hello CFEngine!", + }, + ] + assert generate_augment("create-single-file-with-content", input_data) == { + "variables": { + "cfbs:create_single_file_with_content.filename": { + "value": "/tmp/create-single-file.txt", + "comment": "Added by 'cfbs input'", + }, + "cfbs:create_single_file_with_content.content": { + "value": "Hello CFEngine!", + "comment": "Added by 'cfbs input'", + }, + } + } + + +def test_generate_augment_overridden_defaults(): + """The namespace, bundle, and comment defaults can be overridden""" + input_data = [ + { + "type": "string", + "namespace": "my_namespace", + "bundle": "my_bundle", + "variable": "filename", + "comment": "Example comment.", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + assert generate_augment("create-single-file", input_data) == { + "variables": { + "my_namespace:my_bundle.filename": { + "value": "/tmp/create-single-file.txt", + "comment": "Example comment.", + } + } + } + + +def test_generate_augment_list(): + """The "create multiple files" example from JSON.md + + A list with a subtype of a single value, so the responses are + just strings. + """ + input_data = [ + { + "type": "list", + "variable": "files", + "label": "Files", + "subtype": { + "type": "string", + "label": "Filename", + "question": "What file should this module create?", + }, + "while": "Do you want to create another file?", + "response": [ + "/tmp/create-multiple-files-1.txt", + "/tmp/create-multiple-files-2.txt", + ], + } + ] + assert generate_augment("create-multiple-files", input_data) == { + "variables": { + "cfbs:create_multiple_files.files": { + "value": [ + "/tmp/create-multiple-files-1.txt", + "/tmp/create-multiple-files-2.txt", + ], + "comment": "Added by 'cfbs input'", + } + } + } + + +def test_generate_augment_list_with_keys(): + """The "create multiple files with content" example from JSON.md + + A list with a subtype of multiple values, so the responses are + objects, keyed by the "key" fields of the subtype. + """ + input_data = [ + { + "type": "list", + "variable": "files", + "label": "Files", + "subtype": [ + { + "key": "name", + "type": "string", + "label": "Name", + "question": "What file should this module create?", + }, + { + "key": "content", + "type": "string", + "label": "Content", + "question": "What content should this file have?", + }, + ], + "while": "Do you want to create another file?", + "response": [ + {"name": "/tmp/one.txt", "content": "Hello CFEngine!"}, + {"name": "/tmp/two.txt", "content": "Bye CFEngine!"}, + ], + } + ] + assert generate_augment("create-multiple-files-with-content", input_data) == { + "variables": { + "cfbs:create_multiple_files_with_content.files": { + "value": [ + {"name": "/tmp/one.txt", "content": "Hello CFEngine!"}, + {"name": "/tmp/two.txt", "content": "Bye CFEngine!"}, + ], + "comment": "Added by 'cfbs input'", + } + } + } + + +def test_generate_augment_no_response(): + """Input definitions without a response are skipped""" + input_data = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + }, + { + "type": "string", + "variable": "content", + "label": "Content", + "question": "What content should this file have?", + "response": "Hello CFEngine!", + }, + ] + assert generate_augment("create-single-file-with-content", input_data) == { + "variables": { + "cfbs:create_single_file_with_content.content": { + "value": "Hello CFEngine!", + "comment": "Added by 'cfbs input'", + } + } + } + + del input_data[1]["response"] + assert generate_augment("create-single-file-with-content", input_data) == { + "variables": {} + } + + assert generate_augment("create-single-file", []) == {"variables": {}} + + +def test_generate_augment_not_a_list(): + """Input data which is not a list of input definitions is incomplete""" + assert generate_augment("create-single-file", None) is None + assert generate_augment("create-single-file", {"variable": "filename"}) is None From 8bcd342b698bdd42168cc2b4d3826bba6dccb99d Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 11:09:22 +0200 Subject: [PATCH 3/9] Moved opening of file arguments into a helper in utils.py Generalized the logic in 'get-input' and 'set-input' for opening a file and conditionally returning the stdin/stdout file descriptors. This function will be reused by the render-input command implement in ticket ENT-11346. Signed-off-by: Lars Erik Wik --- cfbs/main.py | 20 ++++++++++---------- cfbs/utils.py | 18 +++++++++++++++++- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/cfbs/main.py b/cfbs/main.py index 83f59262..541bf816 100644 --- a/cfbs/main.py +++ b/cfbs/main.py @@ -4,7 +4,6 @@ __copyright__ = ["Northern.tech AS"] import logging as log -import sys import os import traceback import pathlib @@ -20,6 +19,7 @@ CFBSProgrammerError, CFBSNetworkError, migrate_config_paths, + open_file_arg, ) from cfbs.cfbs_config import CFBSConfig from cfbs import commands @@ -226,20 +226,20 @@ def _main() -> int: module, filename = args.args[0], args.args[1] - if filename == "-": - file = sys.stdin if args.command == "set-input" else sys.stdout - else: - try: - file = open(filename, "r" if args.command == "set-input" else "w") - except OSError as e: - log.error("Can't open '%s': %s" % (filename, e)) - return 1 + try: + file, needs_close = open_file_arg( + filename, "r" if args.command == "set-input" else "w" + ) + except OSError as e: + log.error("Can't open '%s': %s" % (filename, e)) + return 1 try: if args.command == "set-input": return commands.set_input_command(module, file) return commands.get_input_command(module, file) finally: - file.close() + if needs_close: + file.close() raise CFBSProgrammerError( "Command '%s' not handled appropriately by the code above" % args.command diff --git a/cfbs/utils.py b/cfbs/utils.py index 29806886..50a9498e 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -13,7 +13,7 @@ from collections import OrderedDict from pathlib import Path from shutil import rmtree -from typing import Iterable, List, Optional, Tuple, Union +from typing import IO, Iterable, List, Optional, Tuple, Union import filecmp from cfbs.pretty import pretty @@ -296,6 +296,22 @@ def save_file(path, data): f.write(data) +def open_file_arg(filename, mode) -> Tuple[IO, bool]: + """Open a filename given as a command line argument. + + A filename of "-" means stdin or stdout, depending on the mode. + + :param filename: filename from the command line, or "-" + :param mode: "r" to read the file, "w" to write it + :return: (file, needs_close), needs_close being False for stdin and stdout, + since we should not close those + """ + assert mode in ("r", "w") + if filename == "-": + return (sys.stdin if mode == "r" else sys.stdout), False + return open(filename, mode), True + + def read_json(path) -> Union[OrderedDict, None]: try: with open(path, "r") as f: From c52aa40dfde16bb4f4429e83a0b934587952e566 Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 12:50:57 +0200 Subject: [PATCH 4/9] Fixed crash when looking up a module in a project without a build list get_module_from_build() looked up the "build" key unconditionally, raising an exception for a project that uses "provides" instead. Signed-off-by: Lars Erik Wik --- cfbs/cfbs_json.py | 2 +- tests/shell/051_get_input_no_build_list.sh | 53 ++++++++++++++++++++++ tests/shell/all.sh | 1 + 3 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 tests/shell/051_get_input_no_build_list.sh diff --git a/cfbs/cfbs_json.py b/cfbs/cfbs_json.py index 0454bd5d..38504022 100644 --- a/cfbs/cfbs_json.py +++ b/cfbs/cfbs_json.py @@ -215,7 +215,7 @@ def _module_is_in_build(self, module): return "build" in self and module["name"] in (m["name"] for m in self["build"]) def get_module_from_build(self, module): - for m in self["build"]: + for m in self.get("build") or []: if m["name"] == module: return m return None diff --git a/tests/shell/051_get_input_no_build_list.sh b/tests/shell/051_get_input_no_build_list.sh new file mode 100644 index 00000000..387fb21b --- /dev/null +++ b/tests/shell/051_get_input_no_build_list.sh @@ -0,0 +1,53 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf delete-files + +# A cfbs.json for a module, that only uses "provides" (not "build"). Looking up +# a module used to crash with an uncaught KeyError on the missing "build" key: +echo '{ + "name": "example-module", + "type": "module", + "description": "Example module which provides one module", + "provides": { + "example": { + "description": "Example", + "tags": ["example"], + "steps": ["copy example.cf services/autorun/example.cf"] + } + } +}' > cfbs.json + +# Ask for the input of a module in the index, from a project without a +# "build" list: +cfbs get-input delete-files@0.0.1 actual.output +echo '[ + { + "type": "list", + "variable": "files", + "namespace": "delete_files", + "bundle": "delete_files", + "label": "Files", + "subtype": [ + { + "key": "path", + "type": "string", + "label": "Path", + "question": "Path to file" + }, + { + "key": "why", + "type": "string", + "label": "Why", + "question": "Why should this file be deleted?", + "default": "Unknown" + } + ], + "while": "Specify another file you want deleted on your hosts?" + } +]' > expected.output +diff actual.output expected.output diff --git a/tests/shell/all.sh b/tests/shell/all.sh index 71fc5895..f268e274 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -94,6 +94,7 @@ run_test tests/shell/047_absolute_path_modules.sh run_test tests/shell/048_remove_with_dependencies.sh run_test tests/shell/049_remove_with_circular_dependencies.sh run_test tests/shell/050_update_masterfiles_specific_version.sh +run_test tests/shell/051_get_input_no_build_list.sh # Summary _suite_end=$(date +%s) From 16bdc76462a8c08b51fc71001032db238c778564 Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 13:52:07 +0200 Subject: [PATCH 5/9] Fixed crash when looking up a module which is not in the index The get_module_object() function now takes a "default" argument, returned when the module or the requested version is not in the index, defaulting to None. Signed-off-by: Lars Erik Wik --- cfbs/cfbs_config.py | 3 +- cfbs/commands.py | 8 ++++- cfbs/index.py | 20 +++++++++++ tests/shell/052_get_input_module_not_found.sh | 36 +++++++++++++++++++ tests/shell/all.sh | 1 + 5 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 tests/shell/052_get_input_module_not_found.sh diff --git a/cfbs/cfbs_config.py b/cfbs/cfbs_config.py index 63f6a5d2..1b250e22 100644 --- a/cfbs/cfbs_config.py +++ b/cfbs/cfbs_config.py @@ -449,7 +449,8 @@ def _add_modules( ] modules_already_added = self["build"] - assert not any(m for m in modules_to_add if "name" not in m) + # check_existence() above ensures all of them are in the index: + assert not any(m for m in modules_to_add if m is None or "name" not in m) assert not any(m for m in modules_already_added if "name" not in m) # Find all unmet dependencies: diff --git a/cfbs/commands.py b/cfbs/commands.py index 8ca8e83c..c92521d6 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -704,6 +704,9 @@ def update_command(to_update): new_module = provides[module_name] elif is_module_absolute(old_module["name"]): new_module = index.get_module_object(update.name) + # Module objects for absolute modules are generated, not looked up + # in the index, so this is never None: + assert new_module is not None new_module["commit"] = head_commit_hash(old_module["name"]) else: @@ -1269,7 +1272,10 @@ def cfbs_convert_git_commit( raise current_index = CFBSConfig.get_instance().index - default_version = current_index.get_module_object("masterfiles")["version"] + masterfiles = current_index.get_module_object("masterfiles") + if masterfiles is None: + raise CFBSExitError("Could not find the 'masterfiles' module in the index") + default_version = masterfiles["version"] reference_version = analyzed_files.reference_version if reference_version is None: diff --git a/cfbs/index.py b/cfbs/index.py index 41f0e8ca..5cc2fa25 100644 --- a/cfbs/index.py +++ b/cfbs/index.py @@ -214,7 +214,23 @@ def get_module_object( module, added_by: Optional[str] = None, explicit_build_steps: Optional[List[str]] = None, + default=None, ): + """Get the module object for a module in the index. + + Local ("./name") and absolute ("/path/") modules are not in the index, + their module objects are generated with default build steps instead. + + :param module: module name, "name@version", or a Module object + :param added_by: what to put in the module's "added_by" field, omitted + from the module object if not specified + :param explicit_build_steps: build steps to use for a local subdirectory + module, instead of the default "directory" + build step + :param default: what to return if the module, or the requested version + of it, is not in the index + :return: the module object, or default if it was not found + """ if isinstance(module, str): module = Module(module) name = module.name @@ -231,6 +247,8 @@ def get_module_object( # due to that, this hack is used to prevent creating the "version" field module = Module(name).to_dict() else: + if name not in self: + return default object = self[name] if version: try: @@ -239,6 +257,8 @@ def get_module_object( raise CFBSExitError( "Downloading CFEngine Build Module Index failed - check your Wi-Fi / network settings." ) + if name not in versions or version not in versions[name]: + return default new_values = versions[name][version] specifics = { k: v for (k, v) in new_values.items() if k in Module.attributes() diff --git a/tests/shell/052_get_input_module_not_found.sh b/tests/shell/052_get_input_module_not_found.sh new file mode 100644 index 00000000..441abdfc --- /dev/null +++ b/tests/shell/052_get_input_module_not_found.sh @@ -0,0 +1,36 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf create-single-file + +echo '{ + "build": [ + { + "name": "create-single-file", + "input": [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?" + } + ] + } + ] +}' > cfbs.json + +# Asks for the input of a module which is neither in the project nor in the +# index. This used to crash with an uncaught KeyError from the index, instead of +# reporting that the module was not found: +! cfbs get-input no-such-module-anywhere - 2> actual.error +grep "Module 'no-such-module-anywhere' not found" actual.error +! grep "Traceback" actual.error + +# Asks for the input of a module where version does not exist +! cfbs get-input delete-files@9.9.9 - 2> actual.error +grep "Module 'delete-files@9.9.9' not found" actual.error +! grep "Traceback" actual.error diff --git a/tests/shell/all.sh b/tests/shell/all.sh index f268e274..9493334c 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -95,6 +95,7 @@ run_test tests/shell/048_remove_with_dependencies.sh run_test tests/shell/049_remove_with_circular_dependencies.sh run_test tests/shell/050_update_masterfiles_specific_version.sh run_test tests/shell/051_get_input_no_build_list.sh +run_test tests/shell/052_get_input_module_not_found.sh # Summary _suite_end=$(date +%s) From b3910898d9d0c9bcfa5de003517d89267383c80a Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 15:01:53 +0200 Subject: [PATCH 6/9] Fixed crash when input data is not a list of input definitions input_data_matches_spec() zipped the input definition and the input data together without checking that they are lists. Signed-off-by: Lars Erik Wik --- cfbs/validate.py | 3 ++ tests/shell/053_set_input_not_a_list.sh | 40 +++++++++++++++++++++++++ tests/shell/all.sh | 1 + tests/test_validate.py | 21 +++++++++++++ 4 files changed, 65 insertions(+) create mode 100644 tests/shell/053_set_input_not_a_list.sh diff --git a/cfbs/validate.py b/cfbs/validate.py index ca1e3662..dca67ed0 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -815,6 +815,9 @@ def input_data_matches_spec(spec, data): :param data: the input data to check :return: True if the input data conforms with the input definition """ + if not isinstance(spec, list) or not isinstance(data, list): + return False + for a, b in zip(spec, data): if ( not isinstance(a, dict) diff --git a/tests/shell/053_set_input_not_a_list.sh b/tests/shell/053_set_input_not_a_list.sh new file mode 100644 index 00000000..0998d6e2 --- /dev/null +++ b/tests/shell/053_set_input_not_a_list.sh @@ -0,0 +1,40 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf create-single-file + +echo '{ + "build": [ + { + "name": "create-single-file", + "input": [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?" + } + ] + } + ] +}' > cfbs.json + +# Input data which is not a list of input definitions used to crash with an +# uncaught TypeError, instead of reporting that it doesn't conform: +echo '0' > actual.input +! cfbs set-input create-single-file actual.input 2> actual.error +grep "does not conform with input definition" actual.error +! grep "Traceback" actual.error + +# An empty object was silently accepted, since there was nothing to compare: +echo '{}' > actual.input +! cfbs set-input create-single-file actual.input 2> actual.error +grep "does not conform with input definition" actual.error +! grep "Traceback" actual.error + +# None of it was stored in the project: +test ! -e create-single-file/input.json diff --git a/tests/shell/all.sh b/tests/shell/all.sh index 9493334c..a3bd0978 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -96,6 +96,7 @@ run_test tests/shell/049_remove_with_circular_dependencies.sh run_test tests/shell/050_update_masterfiles_specific_version.sh run_test tests/shell/051_get_input_no_build_list.sh run_test tests/shell/052_get_input_module_not_found.sh +run_test tests/shell/053_set_input_not_a_list.sh # Summary _suite_end=$(date +%s) diff --git a/tests/test_validate.py b/tests/test_validate.py index 53962daa..c6ce62df 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -257,3 +257,24 @@ def test_input_data_matches_spec_not_objects(): ] assert not input_data_matches_spec(spec, ["not an object"]) assert not input_data_matches_spec(["not an object"], spec) + + +def test_input_data_matches_spec_not_lists(): + spec = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + } + ] + # Input data has to be a list of input definitions: + assert not input_data_matches_spec(spec, 0) + assert not input_data_matches_spec(spec, "response") + assert not input_data_matches_spec(spec, None) + assert not input_data_matches_spec(spec, {}) + assert not input_data_matches_spec(spec, {"variable": "filename"}) + + # And so does the input definition: + assert not input_data_matches_spec(0, spec) + assert not input_data_matches_spec({}, spec) From 6be2d72761af811ad9d0006862f3a6ed4acf654a Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 15:23:32 +0200 Subject: [PATCH 7/9] Added the 'cfbs render-input' command Converts input data for a module into an augments file (def.json). This is the same conversion the 'input' build step performs during 'cfbs build', except that the input data is read from an infile instead of /input.json, and nothing is stored in the project. Mission Portal needs this to render the augment for module input which is entered per host group, and thus stored in its database rather than in the project. Ticket: ENT-11346 Signed-off-by: Lars Erik Wik --- JSON.md | 3 + README.md | 40 +++++++ cfbs/commands.py | 42 +++++++ cfbs/main.py | 36 ++++++ tests/shell/054_render_input.sh | 56 +++++++++ tests/shell/055_render_input_two_variables.sh | 63 +++++++++++ tests/shell/056_render_input_list.sh | 106 ++++++++++++++++++ tests/shell/057_render_input_no_response.sh | 32 ++++++ tests/shell/058_render_input_fail.sh | 66 +++++++++++ tests/shell/all.sh | 5 + 10 files changed, 449 insertions(+) create mode 100644 tests/shell/054_render_input.sh create mode 100644 tests/shell/055_render_input_two_variables.sh create mode 100644 tests/shell/056_render_input_list.sh create mode 100644 tests/shell/057_render_input_no_response.sh create mode 100644 tests/shell/058_render_input_fail.sh diff --git a/JSON.md b/JSON.md index c9c9840d..7c946ca0 100644 --- a/JSON.md +++ b/JSON.md @@ -477,6 +477,9 @@ Some modules allow for users to add module input by responding to questions expr User input can be added using the `cfbs input ` command, which stores responses in `.//input.json`. These responses are translated into augments which will be added to `./out/masterfiles/def.json` during `cfbs build`. +The `cfbs render-input` command performs the same translation, but writes the augments to a file of your choosing or stdout. +Augments rendered with `cfbs render-input` will not be added to `./out/masterfiles/def.json` automatically during a build. + ### Create single file example The `"input"` attribute takes a list of input definitions as illustrated below. diff --git a/README.md b/README.md index 0190796d..ae1695ba 100644 --- a/README.md +++ b/README.md @@ -150,6 +150,43 @@ Here is an example of an `input.json` file with responses: The `input.json` file is converted and merged into the main `def.json` during the build, using the `input` build step. +### Render module input as an augments file + +``` +cfbs render-input +``` + +Converts input data for a module, as shown above, into the augments (`def.json`) format, and writes it to the outfile. +This is the same conversion the `input` build step performs during `cfbs build`, except that the input data is read from the infile instead of `/input.json`, and nothing is stored in the project. + +Use it to render an augment for input data which is kept outside of the project, for example input entered per host group in Mission Portal: + +``` +$ cfbs render-input create-single-file - - < int: finally: if needs_close: file.close() + if args.command == "render-input": + if len(args.args) != 3: + log.error( + "%s , " + " and " + % ( + "Too many arguments: expected" + if len(args.args) > 3 + else "Missing required arguments" + ) + ) + return 1 + + module, infilename, outfilename = args.args + + # Open the infile first, so that a mistyped infile doesn't truncate the + # outfile: + try: + infile, close_infile = open_file_arg(infilename, "r") + except OSError as e: + log.error("Can't open '%s': %s" % (infilename, e)) + return 1 + try: + outfile, close_outfile = open_file_arg(outfilename, "w") + except OSError as e: + log.error("Can't open '%s': %s" % (outfilename, e)) + if close_infile: + infile.close() + return 1 + try: + return commands.render_input_command(module, infile, outfile) + finally: + if close_infile: + infile.close() + if close_outfile: + outfile.close() raise CFBSProgrammerError( "Command '%s' not handled appropriately by the code above" % args.command diff --git a/tests/shell/054_render_input.sh b/tests/shell/054_render_input.sh new file mode 100644 index 00000000..0c3004c2 --- /dev/null +++ b/tests/shell/054_render_input.sh @@ -0,0 +1,56 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf create-single-file + +echo '{ + "build": [ + { + "name": "create-single-file", + "input": [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?" + } + ] + } + ] +}' > cfbs.json + +cat > expected.output <<'EOF' +{ + "variables": { + "cfbs:create_single_file.filename": { + "value": "/tmp/create-single-file.txt", + "comment": "Added by 'cfbs input'" + } + } +} +EOF + +echo '[ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt" + } +]' > actual.input + +# Render the augment, from stdin to stdout: +cfbs render-input create-single-file - - < actual.input > actual.output +diff actual.output expected.output + +# The same, using files instead of stdin and stdout: +cfbs render-input create-single-file actual.input actual.output +diff actual.output expected.output + +# Rendering the input doesn't store anything in the project: +test ! -e create-single-file/input.json diff --git a/tests/shell/055_render_input_two_variables.sh b/tests/shell/055_render_input_two_variables.sh new file mode 100644 index 00000000..4e18c767 --- /dev/null +++ b/tests/shell/055_render_input_two_variables.sh @@ -0,0 +1,63 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf create-single-file-with-content + +echo '{ + "build": [ + { + "name": "create-single-file-with-content", + "input": [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?" + }, + { + "type": "string", + "variable": "content", + "label": "Content", + "question": "What content should this file have?" + } + ] + } + ] +}' > cfbs.json + +echo '[ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt" + }, + { + "type": "string", + "variable": "content", + "label": "Content", + "question": "What content should this file have?", + "response": "Hello CFEngine!" + } +]' | cfbs render-input create-single-file-with-content - - > actual.output + +cat > expected.output <<'EOF' +{ + "variables": { + "cfbs:create_single_file_with_content.filename": { + "value": "/tmp/create-single-file.txt", + "comment": "Added by 'cfbs input'" + }, + "cfbs:create_single_file_with_content.content": { + "value": "Hello CFEngine!", + "comment": "Added by 'cfbs input'" + } + } +} +EOF +diff actual.output expected.output diff --git a/tests/shell/056_render_input_list.sh b/tests/shell/056_render_input_list.sh new file mode 100644 index 00000000..50d36107 --- /dev/null +++ b/tests/shell/056_render_input_list.sh @@ -0,0 +1,106 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf conditional-installer + +# A module with an explicit namespace and bundle, and a list of responses: +echo '{ + "build": [ + { + "name": "conditional-installer", + "input": [ + { + "type": "string", + "variable": "packages_to_uninstall", + "namespace": "conditional_installer", + "bundle": "main", + "label": "Uninstall", + "question": "Which package(s) would you like to be uninstalled?" + }, + { + "type": "list", + "variable": "packages_to_install", + "namespace": "conditional_installer", + "bundle": "main", + "label": "Install", + "subtype": [ + { + "key": "packages", + "type": "string", + "label": "Package(s)", + "question": "Package(s) to install" + }, + { + "key": "condition", + "type": "string", + "label": "Condition", + "question": "Condition for where to install" + } + ], + "while": "Do you want to specify more packages to be installed?" + } + ] + } + ] +}' > cfbs.json + +echo '[ + { + "type": "string", + "variable": "packages_to_uninstall", + "namespace": "conditional_installer", + "bundle": "main", + "label": "Uninstall", + "question": "Which package(s) would you like to be uninstalled?", + "response": "wget" + }, + { + "type": "list", + "variable": "packages_to_install", + "namespace": "conditional_installer", + "bundle": "main", + "label": "Install", + "subtype": [ + { + "key": "packages", + "type": "string", + "label": "Package(s)", + "question": "Package(s) to install" + }, + { + "key": "condition", + "type": "string", + "label": "Condition", + "question": "Condition for where to install" + } + ], + "while": "Do you want to specify more packages to be installed?", + "response": [ + { "packages": "curl", "condition": "linux" }, + { "packages": "vim", "condition": "any" } + ] + } +]' | cfbs render-input conditional-installer - - > actual.output + +cat > expected.output <<'EOF' +{ + "variables": { + "conditional_installer:main.packages_to_uninstall": { + "value": "wget", + "comment": "Added by 'cfbs input'" + }, + "conditional_installer:main.packages_to_install": { + "value": [ + { "packages": "curl", "condition": "linux" }, + { "packages": "vim", "condition": "any" } + ], + "comment": "Added by 'cfbs input'" + } + } +} +EOF +diff actual.output expected.output diff --git a/tests/shell/057_render_input_no_response.sh b/tests/shell/057_render_input_no_response.sh new file mode 100644 index 00000000..63177be9 --- /dev/null +++ b/tests/shell/057_render_input_no_response.sh @@ -0,0 +1,32 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf create-single-file + +echo '{ + "build": [ + { + "name": "create-single-file", + "input": [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?" + } + ] + } + ] +}' > cfbs.json + +# Input data straight from 'cfbs get-input', i.e. without any responses, renders +# an augment without any variables: +cfbs get-input create-single-file - | cfbs render-input create-single-file - - > actual.output +echo '{ + "variables": {} +}' > expected.output +diff actual.output expected.output diff --git a/tests/shell/058_render_input_fail.sh b/tests/shell/058_render_input_fail.sh new file mode 100644 index 00000000..dae87dbb --- /dev/null +++ b/tests/shell/058_render_input_fail.sh @@ -0,0 +1,66 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf create-single-file + +echo '{ + "build": [ + { + "name": "create-single-file", + "input": [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?" + } + ] + }, + { + "name": "autorun" + } + ] +}' > cfbs.json + +# A changed value doesn't conform with the input definition: +echo '[ + { + "type": "string", + "variable": "bogus", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt" + } +]' > actual.input +! cfbs render-input create-single-file actual.input - + +# Neither does a renamed key: +echo '[ + { + "doofus": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt" + } +]' > actual.input +! cfbs render-input create-single-file actual.input - + +# Input data which doesn't parse as json: +echo 'not json' > actual.input +! cfbs render-input create-single-file actual.input - + +# A module which doesn't accept any input: +echo '[]' > actual.input +! cfbs render-input autorun actual.input - + +# A module which doesn't exist: +! cfbs render-input no-such-module-anywhere actual.input - + +# A missing outfile, and one argument too many: +! cfbs render-input create-single-file actual.input +! cfbs render-input create-single-file actual.input - - diff --git a/tests/shell/all.sh b/tests/shell/all.sh index a3bd0978..0fe315cd 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -97,6 +97,11 @@ run_test tests/shell/050_update_masterfiles_specific_version.sh run_test tests/shell/051_get_input_no_build_list.sh run_test tests/shell/052_get_input_module_not_found.sh run_test tests/shell/053_set_input_not_a_list.sh +run_test tests/shell/054_render_input.sh +run_test tests/shell/055_render_input_two_variables.sh +run_test tests/shell/056_render_input_list.sh +run_test tests/shell/057_render_input_no_response.sh +run_test tests/shell/058_render_input_fail.sh # Summary _suite_end=$(date +%s) From e28ead68e8ae6084fcc9116b1962dd484eda5c2e Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Tue, 28 Jul 2026 16:16:55 +0200 Subject: [PATCH 8/9] Fixed non-deterministic key order in generated augments Signed-off-by: Lars Erik Wik --- cfbs/augments.py | 16 +++++++++++----- tests/test_augments.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/cfbs/augments.py b/cfbs/augments.py index 60d87285..d19b8ca0 100644 --- a/cfbs/augments.py +++ b/cfbs/augments.py @@ -2,6 +2,8 @@ Functions for generating CFEngine augments (def.json) """ +from collections import OrderedDict + from cfbs.utils import canonify @@ -16,7 +18,11 @@ def generate_augment(module_name, input_data): if not isinstance(input_data, list): return None - augment = {"variables": {}} + # OrderedDict, so that the keys are in the same order regardless of + # the Python version. Dictionaries don't preserve the insertion order + # before Python 3.7: + augment = OrderedDict() + augment["variables"] = OrderedDict() for variable in input_data: if not isinstance(variable, dict) or any( @@ -30,9 +36,9 @@ def generate_augment(module_name, input_data): value = variable["response"] comment = variable.get("comment", "Added by 'cfbs input'") - augment["variables"]["%s:%s.%s" % (namespace, bundle, name)] = { - "value": value, - "comment": comment, - } + augment_variable = OrderedDict() + augment_variable["value"] = value + augment_variable["comment"] = comment + augment["variables"]["%s:%s.%s" % (namespace, bundle, name)] = augment_variable return augment diff --git a/tests/test_augments.py b/tests/test_augments.py index 59c8b044..4165f2fc 100644 --- a/tests/test_augments.py +++ b/tests/test_augments.py @@ -1,4 +1,5 @@ from cfbs.augments import generate_augment +from cfbs.pretty import pretty def test_generate_augment_string(): @@ -193,6 +194,33 @@ def test_generate_augment_no_response(): assert generate_augment("create-single-file", []) == {"variables": {}} +def test_generate_augment_key_order(): + """The keys are in the same order regardless of the Python version + + Dictionaries don't preserve the insertion order before Python 3.7, + so the augment is built using OrderedDict. + """ + input_data = [ + { + "type": "string", + "variable": "filename", + "label": "Filename", + "question": "What file should this module create?", + "response": "/tmp/create-single-file.txt", + } + ] + assert pretty(generate_augment("create-single-file", input_data)) == ( + "{\n" + ' "variables": {\n' + ' "cfbs:create_single_file.filename": {\n' + ' "value": "/tmp/create-single-file.txt",\n' + ' "comment": "Added by \'cfbs input\'"\n' + " }\n" + " }\n" + "}" + ) + + def test_generate_augment_not_a_list(): """Input data which is not a list of input definitions is incomplete""" assert generate_augment("create-single-file", None) is None From c588d1f4a10cef113ac1f96aa89d795d4e8d0ccf Mon Sep 17 00:00:00 2001 From: Lars Erik Wik Date: Wed, 29 Jul 2026 14:51:11 +0200 Subject: [PATCH 9/9] Made open_file_arg a context manager Signed-off-by: Lars Erik Wik Co-authored-by: Ole Herman Schumacher Elgesem <4048546+olehermanse@users.noreply.github.com> --- cfbs/main.py | 59 ++++++++++++++++++++++++--------------------------- cfbs/utils.py | 17 +++++++++------ 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/cfbs/main.py b/cfbs/main.py index 6063e8d0..f50fac06 100644 --- a/cfbs/main.py +++ b/cfbs/main.py @@ -3,6 +3,7 @@ __copyright__ = ["Northern.tech AS"] +import contextlib import logging as log import os import traceback @@ -226,20 +227,20 @@ def _main() -> int: module, filename = args.args[0], args.args[1] - try: - file, needs_close = open_file_arg( - filename, "r" if args.command == "set-input" else "w" - ) - except OSError as e: - log.error("Can't open '%s': %s" % (filename, e)) - return 1 - try: + # ExitStack rather than a plain with statement, so that the OSError + # handling only covers opening the files, not whatever exceptions happen + # in commands.set_input() / commands.get_input(): + with contextlib.ExitStack() as stack: + try: + file = stack.enter_context( + open_file_arg(filename, "r" if args.command == "set-input" else "w") + ) + except OSError as e: + log.error("Can't open '%s': %s" % (filename, e)) + return 1 if args.command == "set-input": return commands.set_input_command(module, file) return commands.get_input_command(module, file) - finally: - if needs_close: - file.close() if args.command == "render-input": if len(args.args) != 3: log.error( @@ -255,27 +256,23 @@ def _main() -> int: module, infilename, outfilename = args.args - # Open the infile first, so that a mistyped infile doesn't truncate the - # outfile: - try: - infile, close_infile = open_file_arg(infilename, "r") - except OSError as e: - log.error("Can't open '%s': %s" % (infilename, e)) - return 1 - try: - outfile, close_outfile = open_file_arg(outfilename, "w") - except OSError as e: - log.error("Can't open '%s': %s" % (outfilename, e)) - if close_infile: - infile.close() - return 1 - try: + # ExitStack rather than a plain with statement, so that the OSError + # handling only covers opening the files, not whatever exceptions happen + # in commands.render_input_command(): + with contextlib.ExitStack() as stack: + # Open the infile first, so that a mistyped infile doesn't truncate + # the outfile: + try: + infile = stack.enter_context(open_file_arg(infilename, "r")) + except OSError as e: + log.error("Can't open '%s': %s" % (infilename, e)) + return 1 + try: + outfile = stack.enter_context(open_file_arg(outfilename, "w")) + except OSError as e: + log.error("Can't open '%s': %s" % (outfilename, e)) + return 1 return commands.render_input_command(module, infile, outfile) - finally: - if close_infile: - infile.close() - if close_outfile: - outfile.close() raise CFBSProgrammerError( "Command '%s' not handled appropriately by the code above" % args.command diff --git a/cfbs/utils.py b/cfbs/utils.py index 50a9498e..15730191 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -11,9 +11,10 @@ import urllib.request # needed on some platforms import urllib.error from collections import OrderedDict +from contextlib import contextmanager from pathlib import Path from shutil import rmtree -from typing import IO, Iterable, List, Optional, Tuple, Union +from typing import IO, Iterable, Iterator, List, Optional, Tuple, Union import filecmp from cfbs.pretty import pretty @@ -296,20 +297,22 @@ def save_file(path, data): f.write(data) -def open_file_arg(filename, mode) -> Tuple[IO, bool]: +@contextmanager +def open_file_arg(filename, mode) -> Iterator[IO]: """Open a filename given as a command line argument. - A filename of "-" means stdin or stdout, depending on the mode. + A filename of "-" means stdin or stdout, depending on the mode. stdin and + stdout are not closed on exit, since we should not close those. :param filename: filename from the command line, or "-" :param mode: "r" to read the file, "w" to write it - :return: (file, needs_close), needs_close being False for stdin and stdout, - since we should not close those """ assert mode in ("r", "w") if filename == "-": - return (sys.stdin if mode == "r" else sys.stdout), False - return open(filename, mode), True + yield sys.stdin if mode == "r" else sys.stdout + return + with open(filename, mode) as f: + yield f def read_json(path) -> Union[OrderedDict, None]: