From 58347dd8d4a92332720467e39d4b1e0c11c84621 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 3 Jul 2025 14:48:24 +0200 Subject: [PATCH 1/5] Moved out build step validation function for reuse Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/validate.py | 51 +++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/cfbs/validate.py b/cfbs/validate.py index 3f9fa553..1070462f 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -130,6 +130,31 @@ def validate_config(config, empty_build_list_ok=False): return 1 +def validate_build_step(name, i, operation, args): + if not operation in AVAILABLE_BUILD_STEPS: + raise CFBSValidationError( + name, + 'Unknown operation "%s" in "steps", must be one of: %s (build step %s in module "%s")' + % (operation, ", ".join(AVAILABLE_BUILD_STEPS), i, name), + ) + expected = AVAILABLE_BUILD_STEPS[operation] + actual = len(args) + if not step_has_valid_arg_count(args, expected): + if type(expected) is int: + raise CFBSValidationError( + name, + "The %s build step expects %d arguments, %d were given (build step " + % (operation, expected, actual), + ) + else: + expected = int(expected[0:-1]) + raise CFBSValidationError( + name, + "The %s build step expects %d or more arguments, %d were given" + % (operation, expected, actual), + ) + + def _validate_module_object(context, name, module, config): def validate_alias(name, module, context): if context == "index": @@ -261,7 +286,7 @@ def validate_steps(name, module): raise CFBSValidationError(name, '"steps" must be of type list') if not module["steps"]: raise CFBSValidationError(name, '"steps" must be non-empty') - for step in module["steps"]: + for i, step in enumerate(module["steps"]): if type(step) != str: raise CFBSValidationError(name, '"steps" must be a list of strings') if not step or step.strip() == "": @@ -269,29 +294,7 @@ def validate_steps(name, module): name, '"steps" must be a list of non-empty / non-whitespace strings' ) operation, args = split_build_step(step) - if not operation in AVAILABLE_BUILD_STEPS: - x = ", ".join(AVAILABLE_BUILD_STEPS) - raise CFBSValidationError( - name, - 'Unknown operation "%s" in "steps", must be one of: (%s)' - % (operation, x), - ) - expected = AVAILABLE_BUILD_STEPS[operation] - actual = len(args) - if not step_has_valid_arg_count(args, expected): - if type(expected) is int: - raise CFBSValidationError( - name, - "The %s build step expects %d arguments, %d were given" - % (operation, expected, actual), - ) - else: - expected = int(expected[0:-1]) - raise CFBSValidationError( - name, - "The %s build step expects %d or more arguments, %d were given" - % (operation, expected, actual), - ) + validate_build_step(name, i, operation, args) def validate_url_field(name, module, field): assert field in module From 4f3eafc834d30d8be89b8d479bfc88fe6023a6bf Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 3 Jul 2025 02:37:45 +0200 Subject: [PATCH 2/5] Added replace build step Signed-off-by: Ole Herman Schumacher Elgesem --- JSON.md | 6 +++ cfbs/build.py | 51 +++++++++++++++++++ tests/shell/044_replace.sh | 21 ++++++++ tests/shell/044_replace/example-cfbs.json | 19 +++++++ .../044_replace/subdir/example.expected.py | 4 ++ tests/shell/044_replace/subdir/example.py | 4 ++ tests/shell/all.sh | 1 + 7 files changed, 106 insertions(+) create mode 100644 tests/shell/044_replace.sh create mode 100644 tests/shell/044_replace/example-cfbs.json create mode 100644 tests/shell/044_replace/subdir/example.expected.py create mode 100644 tests/shell/044_replace/subdir/example.py diff --git a/JSON.md b/JSON.md index 0b61ff1d..7e986040 100644 --- a/JSON.md +++ b/JSON.md @@ -264,6 +264,12 @@ In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest - Converts the input data for a module into the augments format and merges it with the target augments file. - Source is relative to module directory and target is relative to `out/masterfiles`. - In most cases, the build step should be: `input ./input.json def.json` +- `replace ` + - Replace string `` with string ``, exactly `` times, in file `filename`. + - The number of occurences is strict: It will error if the string cannot be found, cannot be replaced exactly `` times, or can still be found after replacements are done. + (This is to try to catch mistakes). + - `n` must be an integer, greater than 0, and may optionally have a trailing `+` to signify "or more". + - string `` must not contain string ``, as that could lead to confusing / recursive replacement situations. - `replace_version ` - Replace the string inside the file with the version number of that module. - The module must have a version and the string must occur exactly once in the file. diff --git a/cfbs/build.py b/cfbs/build.py index 7c8edca7..e69db818 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -30,6 +30,7 @@ "input": 2, "policy_files": "1+", "bundles": "1+", + "replace": 4, # n, a, b, filename "replace_version": 2, # string to replace and filename } @@ -97,6 +98,50 @@ def step_has_valid_arg_count(args, expected): return True +def _perform_replace_step(n, a, b, filename): + or_more = False + if n.endswith("+"): + n = n[0:-1] + or_more = True + n = int(n) + if n <= 0 or n > 1000: + user_error("replace build step cannot replace something %s times" % (n)) + if a in b and (n >= 2 or or_more): + user_error( + "'%s' must not contain '%s' (could lead to recursive replacing)" % (a, b) + ) + if not os.path.isfile(filename): + user_error("No such file '%s' in replace build step" % (filename,)) + try: + with open(filename, "r") as f: + content = f.read() + except: + user_error("Could not open/read '%s' in replace build step" % (filename,)) + new_content = previous_content = content + for i in range(0, n): + previous_content = new_content + new_content = previous_content.replace(a, b, 1) + if new_content == previous_content: + user_error( + "replace build step could only replace '%s' in '%s' %s times, not %s times (required)" + % (a, filename, i, n) + ) + + if or_more: + for i in range(n, 1000): + previous_content = new_content + new_content = previous_content.replace(a, b, 1) + if new_content == previous_content: + break + if a in new_content: + user_error("too many occurences of '%s' in '%s'" % (a, filename)) + try: + with open(filename, "w") as f: + f.write(new_content) + except: + user_error("Failed to write to '%s'" % (filename,)) + + def _perform_build_step(module, step, max_length): operation, args = split_build_step(step) source = module["_directory"] @@ -260,6 +305,12 @@ def _perform_build_step(module, step, max_length): merged = augment log.debug("Merged def.json: %s", pretty(merged)) write_json(path, merged) + elif operation == "replace": + assert len(args) == 4 + print("%s replace '%s'" % (prefix, "' '".join(args))) + n, a, b, file = args + file = os.path.join(destination, file) + _perform_replace_step(n, a, b, file) elif operation == "replace_version": assert len(args) == 2 print("%s replace_version '%s'" % (prefix, "' '".join(args))) diff --git a/tests/shell/044_replace.sh b/tests/shell/044_replace.sh new file mode 100644 index 00000000..0bbd18bc --- /dev/null +++ b/tests/shell/044_replace.sh @@ -0,0 +1,21 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ + +# Set up the project we will build: +cp ../shell/044_replace/example-cfbs.json ./cfbs.json +mkdir -p subdir +cp ../shell/044_replace/subdir/example.py ./subdir/example.py +cp ../shell/044_replace/subdir/example.expected.py ./subdir/example.expected.py + +cfbs build + +ls out/masterfiles/services/cfbs/subdir/example.py + +# Replace should have changed it: +! diff ./subdir/example.py out/masterfiles/services/cfbs/subdir/example.py > /dev/null + +# This is the expected content: +diff ./subdir/example.expected.py out/masterfiles/services/cfbs/subdir/example.py diff --git a/tests/shell/044_replace/example-cfbs.json b/tests/shell/044_replace/example-cfbs.json new file mode 100644 index 00000000..98a66c1e --- /dev/null +++ b/tests/shell/044_replace/example-cfbs.json @@ -0,0 +1,19 @@ +{ + "name": "Example project", + "description": "Example description", + "type": "policy-set", + "git": true, + "build": [ + { + "name": "./subdir/", + "description": "Local subdirectory added using cfbs command line", + "added_by": "cfbs add", + "steps": [ + "copy example.py services/cfbs/subdir/example.py", + "replace 1 foo bar services/cfbs/subdir/example.py", + "replace 2 alice bob services/cfbs/subdir/example.py", + "replace 1+ lorem ipsum services/cfbs/subdir/example.py" + ] + } + ] +} diff --git a/tests/shell/044_replace/subdir/example.expected.py b/tests/shell/044_replace/subdir/example.expected.py new file mode 100644 index 00000000..acaa90af --- /dev/null +++ b/tests/shell/044_replace/subdir/example.expected.py @@ -0,0 +1,4 @@ +if __name__ == "__main__": + print("bar") + print("bob,bob") + print("ipsum,ipsum,ipsum") diff --git a/tests/shell/044_replace/subdir/example.py b/tests/shell/044_replace/subdir/example.py new file mode 100644 index 00000000..bba3a1cd --- /dev/null +++ b/tests/shell/044_replace/subdir/example.py @@ -0,0 +1,4 @@ +if __name__ == "__main__": + print("foo") + print("alice,alice") + print("lorem,lorem,lorem") diff --git a/tests/shell/all.sh b/tests/shell/all.sh index ed6c5272..ab083a0e 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -47,5 +47,6 @@ bash tests/shell/040_add_added_by_field_update_2.sh bash tests/shell/041_add_multidep.sh bash tests/shell/042_update_from_url.sh bash tests/shell/043_replace_version.sh +bash tests/shell/044_replace.sh echo "All cfbs shell tests completed successfully!" From dc1d12adcbc7d64a8341b6588b5a0ffee0292c17 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 3 Jul 2025 15:10:42 +0200 Subject: [PATCH 3/5] Moved code from build to validate to avoid circular imports I want to start using some validaiton code inside of build.py This can be problematic due to circular imports (especially the global constants). Thus, it makes more sense to put the common code in validate, and say that build depends on validate, not the other way around. Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/build.py | 43 +++++-------------------------------------- cfbs/validate.py | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 39 deletions(-) diff --git a/cfbs/build.py b/cfbs/build.py index e69db818..9e4d40f3 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -1,7 +1,6 @@ import os import logging as log import shutil -from typing import List, Tuple from cfbs.utils import ( canonify, cp, @@ -19,20 +18,11 @@ write_json, ) from cfbs.pretty import pretty, pretty_file - -AVAILABLE_BUILD_STEPS = { - "copy": 2, - "run": "1+", - "delete": "1+", - "json": 2, - "append": 2, - "directory": 2, - "input": 2, - "policy_files": "1+", - "bundles": "1+", - "replace": 4, # n, a, b, filename - "replace_version": 2, # string to replace and filename -} +from cfbs.validate import ( + AVAILABLE_BUILD_STEPS, + step_has_valid_arg_count, + split_build_step, +) def init_out_folder(): @@ -75,29 +65,6 @@ def _generate_augment(module_name, input_data): return augment -def split_build_step(command) -> Tuple[str, List[str]]: - terms = command.split(" ") - operation, args = terms[0], terms[1:] - return operation, args - - -def step_has_valid_arg_count(args, expected): - actual = len(args) - - if type(expected) is int: - if actual != expected: - return False - - else: - # Only other option is a string of 1+, 2+ or similar: - assert type(expected) is str and expected.endswith("+") - expected = int(expected[0:-1]) - if actual < expected: - return False - - return True - - def _perform_replace_step(n, a, b, filename): or_more = False if n.endswith("+"): diff --git a/cfbs/validate.py b/cfbs/validate.py index 1070462f..b0754f45 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -2,6 +2,7 @@ import sys import re from collections import OrderedDict +from typing import List, Tuple from cfbs.utils import ( is_a_commit_hash, @@ -9,7 +10,43 @@ ) from cfbs.pretty import TOP_LEVEL_KEYS, MODULE_KEYS from cfbs.cfbs_config import CFBSConfig -from cfbs.build import AVAILABLE_BUILD_STEPS, step_has_valid_arg_count, split_build_step + +AVAILABLE_BUILD_STEPS = { + "copy": 2, + "run": "1+", + "delete": "1+", + "json": 2, + "append": 2, + "directory": 2, + "input": 2, + "policy_files": "1+", + "bundles": "1+", + "replace": 4, # n, a, b, filename + "replace_version": 2, # string to replace and filename +} + + +def split_build_step(command) -> Tuple[str, List[str]]: + terms = command.split(" ") + operation, args = terms[0], terms[1:] + return operation, args + + +def step_has_valid_arg_count(args, expected): + actual = len(args) + + if type(expected) is int: + if actual != expected: + return False + + else: + # Only other option is a string of 1+, 2+ or similar: + assert type(expected) is str and expected.endswith("+") + expected = int(expected[0:-1]) + if actual < expected: + return False + + return True class CFBSValidationError(Exception): From 08dea02d08a5b18058ede75d531c881269cf0051 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 3 Jul 2025 15:38:07 +0200 Subject: [PATCH 4/5] Added docstrings to build.py and validate.py Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/build.py | 12 ++++++++++++ cfbs/validate.py | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/cfbs/build.py b/cfbs/build.py index 9e4d40f3..112b528b 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -1,3 +1,15 @@ +""" +Functions for performing the core part of 'cfbs build' + +This module contains the code for performing the actual build, +converting a project into a ready to deploy policy set. +To achieve this, we iterate over all the build steps in all +the modules running the appropriate file and shell operations. + +There are some preliminary parts of 'cfbs build' implemented +elsewhere, like validation and downloading modules. +""" + import os import logging as log import shutil diff --git a/cfbs/validate.py b/cfbs/validate.py index b0754f45..d2e37fd8 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -1,3 +1,24 @@ +""" +Functions for performing the core part of 'cfbs validate' + +Iterate over the JSON structure from cfbs.json, and check +the contents against validation rules. + +Currently, we are not very strict with validation in other +commands, when you run something like 'cfbs build', +many things only produce warnings. This is for backwards +compatibility and we might choose to turn those warnings +into errors in the future. + +Be careful about introducing dependencies to other parts +of the codebase, such as build.py - We want validate.py +to be relatively easy to reuse in various places without +accidentally introducing circular dependencies. +Thus, for example, the common parts needed by both build.py +and validate.py, should be in utils.py or validate.py, +not in build.py. +""" + import argparse import sys import re From ffb3b3417dbe48944896daf3adf936d75b081ad7 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 3 Jul 2025 15:55:46 +0200 Subject: [PATCH 5/5] replace: Better clarified maximum number of replacements Signed-off-by: Ole Herman Schumacher Elgesem --- JSON.md | 7 +++++-- cfbs/build.py | 10 ++++++++-- cfbs/validate.py | 2 ++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/JSON.md b/JSON.md index 7e986040..0c5ac4ca 100644 --- a/JSON.md +++ b/JSON.md @@ -265,11 +265,14 @@ In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest - Source is relative to module directory and target is relative to `out/masterfiles`. - In most cases, the build step should be: `input ./input.json def.json` - `replace ` + - Replace string `` with string ``, exactly `` times, in file `filename`. + - string `` must not contain string ``, as that could lead to confusing / recursive replacement situations. - The number of occurences is strict: It will error if the string cannot be found, cannot be replaced exactly `` times, or can still be found after replacements are done. (This is to try to catch mistakes). - - `n` must be an integer, greater than 0, and may optionally have a trailing `+` to signify "or more". - - string `` must not contain string ``, as that could lead to confusing / recursive replacement situations. + - `n` must be an integer, from 1 to 1000, and may optionally have a trailing `+` to signify "or more". + At most 1000 replacements will be performed, regardless of whether you specify `+` or not. + - `replace_version ` - Replace the string inside the file with the version number of that module. - The module must have a version and the string must occur exactly once in the file. diff --git a/cfbs/build.py b/cfbs/build.py index 112b528b..3e6994a0 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -32,6 +32,7 @@ from cfbs.pretty import pretty, pretty_file from cfbs.validate import ( AVAILABLE_BUILD_STEPS, + MAX_REPLACEMENTS, step_has_valid_arg_count, split_build_step, ) @@ -83,8 +84,13 @@ def _perform_replace_step(n, a, b, filename): n = n[0:-1] or_more = True n = int(n) - if n <= 0 or n > 1000: + if n <= 0: user_error("replace build step cannot replace something %s times" % (n)) + if n > MAX_REPLACEMENTS or n == MAX_REPLACEMENTS and or_more: + user_error( + "replace build step cannot replace something more than %s times" + % (MAX_REPLACEMENTS) + ) if a in b and (n >= 2 or or_more): user_error( "'%s' must not contain '%s' (could lead to recursive replacing)" % (a, b) @@ -107,7 +113,7 @@ def _perform_replace_step(n, a, b, filename): ) if or_more: - for i in range(n, 1000): + for i in range(n, MAX_REPLACEMENTS): previous_content = new_content new_content = previous_content.replace(a, b, 1) if new_content == previous_content: diff --git a/cfbs/validate.py b/cfbs/validate.py index d2e37fd8..d6bf1864 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -46,6 +46,8 @@ "replace_version": 2, # string to replace and filename } +MAX_REPLACEMENTS = 1000 + def split_build_step(command) -> Tuple[str, List[str]]: terms = command.split(" ")