From 53adcbd2a6d1952e2a45c5959f27c410498c88e7 Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Tue, 17 Jun 2025 15:59:52 +0200 Subject: [PATCH 1/5] Replaced string manipulation utility functions with their standard library implementations Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/utils.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cfbs/utils.py b/cfbs/utils.py index 60aada8e..765f0324 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -79,12 +79,12 @@ def cp(src, dst): sh("rsync -r %s/ %s" % (src, dst)) -def pad_left(s, n) -> int: - return s if len(s) >= n else " " * (n - len(s)) + s +def pad_left(s, n): + return s.rjust(n) -def pad_right(s, n) -> int: - return s if len(s) >= n else s + " " * (n - len(s)) +def pad_right(s, n): + return s.ljust(n) def split_command(command) -> Tuple[str, List[str]]: @@ -140,12 +140,14 @@ def item_index(iterable, item, extra_at_end=True): def strip_right(string, ending): + # can be replaced with str.removesuffix from Python 3.9 onwards if not string.endswith(ending): return string return string[0 : -len(ending)] def strip_left(string, beginning): + # can be replaced with str.removeprefix from Python 3.9 onwards if not string.startswith(beginning): return string return string[len(beginning) :] From 3c9b64acd2218885c8b22a5d3f1eb9ab36b42698 Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Tue, 17 Jun 2025 16:04:40 +0200 Subject: [PATCH 2/5] Made directory enumeration utility functions deterministic by sorting Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/utils.py | 14 ++++++++++++-- tests/test_utils.py | 12 ++---------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/cfbs/utils.py b/cfbs/utils.py index 765f0324..d5f06abd 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -277,11 +277,21 @@ def is_cfbs_repo() -> bool: def immediate_subdirectories(path): - return [f.name for f in os.scandir(path) if f.is_dir()] + l = [f.name for f in os.scandir(path) if f.is_dir()] + + # `os.scandir` returns the entries in arbitrary order, so sort for determinism + l = sorted(l) + + return l def immediate_files(path): - return [f.name for f in os.scandir(path) if not f.is_dir()] + l = [f.name for f in os.scandir(path) if not f.is_dir()] + + # `os.scandir` returns the entries in arbitrary order, so sort for determinism + l = sorted(l) + + return l def path_append(dir, subdir): diff --git a/tests/test_utils.py b/tests/test_utils.py index 1a6ebe37..328bd59c 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -230,22 +230,14 @@ def test_immediate_subdirectories(): path = "tests/sample/sample_dir" expected = ["sample_subdir_A", "sample_subdir_B"] - actual = immediate_subdirectories(path) - # `immediate_subdirectories` currently returns the entries in arbitrary order - actual = sorted(actual) - - assert actual == expected + assert immediate_subdirectories(path) == expected def test_immediate_files(): path = "tests/sample/sample_dir" expected = ["sample_file_1.txt", "sample_file_2.txt"] - actual = immediate_files(path) - # `immediate_files` currently returns the entries in arbitrary order - actual = sorted(actual) - - assert actual == expected + assert immediate_files(path) == expected def test_path_append(): From 9a9fd154cf4b65fdafd98b66ccc866908212e244 Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Tue, 17 Jun 2025 16:12:11 +0200 Subject: [PATCH 3/5] Decoupled utility functions related to appending paths and `.cfengine` home directory so that \`path_append\` function name is not misleading Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- cfbs/utils.py | 9 +++++---- tests/test_utils.py | 5 ----- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/cfbs/utils.py b/cfbs/utils.py index d5f06abd..fcd72fef 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -5,7 +5,6 @@ import copy import subprocess import hashlib -import logging as log from typing import List, Tuple import urllib import urllib.request # needed on some platforms @@ -168,7 +167,7 @@ def save_file(path, data): f.write(data) -def read_json(path): +def read_json(path) -> OrderedDict: try: with open(path, "r") as f: return json.loads(f.read(), object_pairs_hook=OrderedDict) @@ -295,7 +294,6 @@ def immediate_files(path): def path_append(dir, subdir): - dir = os.path.abspath(os.path.expanduser(dir)) return dir if not subdir else os.path.join(dir, subdir) @@ -311,7 +309,10 @@ def are_paths_equal(path_a, path_b) -> bool: def cfengine_dir(subdir=None): - return path_append("~/.cfengine/", subdir) + CFENGINE_DIR = "~/.cfengine/" + cfengine_dir_abspath = os.path.abspath(os.path.expanduser(CFENGINE_DIR)) + + return path_append(cfengine_dir_abspath, subdir) def cfbs_dir(append=None) -> str: diff --git a/tests/test_utils.py b/tests/test_utils.py index 328bd59c..cc5bb6bd 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -243,11 +243,6 @@ def test_immediate_files(): def test_path_append(): path = "tests/sample/sample_dir" - # `path_append` is currently coupled with the below code - import os - - path = os.path.abspath(os.path.expanduser(path)) - assert path_append(path, "abc") == path + "/abc" assert path_append(path, None) == path From d94bcb50514549c6c1eabe4fdc57a0f33680b1e9 Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Wed, 18 Jun 2025 15:26:58 +0200 Subject: [PATCH 4/5] Moved and renamed functions and clarified documentation related to build step splitting Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- JSON.md | 2 +- cfbs/build.py | 32 +++++++++++++++++++++++++++----- cfbs/utils.py | 26 +------------------------- cfbs/validate.py | 12 +++++++----- 4 files changed, 36 insertions(+), 36 deletions(-) diff --git a/JSON.md b/JSON.md index 4f861efc..14086709 100644 --- a/JSON.md +++ b/JSON.md @@ -223,7 +223,7 @@ These are copies of the module directories, where it's more "safe" to do things ## All available build steps The build steps below manipulate the temporary files in the steps directories and write results to the output policy set, in `out/masterfiles`. -Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`). +Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`). In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest of the build step by a regular space. - `copy ` - Copy a single file or a directory recursively. diff --git a/cfbs/build.py b/cfbs/build.py index ebfc1121..9b8ac196 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -1,18 +1,17 @@ import os import logging as log +from typing import List, Tuple from cfbs.utils import ( canonify, cp, deduplicate_def_json, find, - is_valid_arg_count, merge_json, mkdir, pad_right, read_json, rm, sh, - split_command, strip_left, touch, user_error, @@ -73,8 +72,31 @@ 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_build_step(module, step, max_length): - operation, args = split_command(step) + operation, args = split_build_step(step) source = module["_directory"] counter = module["_counter"] destination = "out/masterfiles" @@ -245,7 +267,7 @@ def perform_build_steps(config) -> int: # mini-validation for module in config.get("build", []): for step in module["steps"]: - operation, args = split_command(step) + operation, args = split_build_step(step) if step.split() != [operation] + args: user_error( @@ -258,7 +280,7 @@ def perform_build_steps(config) -> int: expected = AVAILABLE_BUILD_STEPS[operation] actual = len(args) - if not is_valid_arg_count(args, expected): + if not step_has_valid_arg_count(args, expected): if type(expected) is int: user_error( "The `%s` build step expects %d arguments, %d were given" diff --git a/cfbs/utils.py b/cfbs/utils.py index fcd72fef..81cfe335 100644 --- a/cfbs/utils.py +++ b/cfbs/utils.py @@ -5,7 +5,6 @@ import copy import subprocess import hashlib -from typing import List, Tuple import urllib import urllib.request # needed on some platforms from collections import OrderedDict @@ -86,29 +85,6 @@ def pad_right(s, n): return s.ljust(n) -def split_command(command) -> Tuple[str, List[str]]: - terms = command.split(" ") - operation, args = terms[0], terms[1:] - return operation, args - - -def is_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 user_error(msg: str): sys.exit("Error: " + msg) @@ -176,7 +152,7 @@ def read_json(path) -> OrderedDict: except NotADirectoryError: return None except json.decoder.JSONDecodeError as ex: - print("Error reading json file {} : {}".format(path, ex)) + print("Error reading json file '{}': {}".format(path, ex)) sys.exit(1) diff --git a/cfbs/validate.py b/cfbs/validate.py index 5bc7751e..3f9fa553 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -1,13 +1,15 @@ import argparse -import json import sys import re from collections import OrderedDict -from cfbs.utils import is_valid_arg_count, is_a_commit_hash, split_command, user_error +from cfbs.utils import ( + is_a_commit_hash, + user_error, +) from cfbs.pretty import TOP_LEVEL_KEYS, MODULE_KEYS from cfbs.cfbs_config import CFBSConfig -from cfbs.build import AVAILABLE_BUILD_STEPS +from cfbs.build import AVAILABLE_BUILD_STEPS, step_has_valid_arg_count, split_build_step class CFBSValidationError(Exception): @@ -266,7 +268,7 @@ def validate_steps(name, module): raise CFBSValidationError( name, '"steps" must be a list of non-empty / non-whitespace strings' ) - operation, args = split_command(step) + operation, args = split_build_step(step) if not operation in AVAILABLE_BUILD_STEPS: x = ", ".join(AVAILABLE_BUILD_STEPS) raise CFBSValidationError( @@ -276,7 +278,7 @@ def validate_steps(name, module): ) expected = AVAILABLE_BUILD_STEPS[operation] actual = len(args) - if not is_valid_arg_count(args, expected): + if not step_has_valid_arg_count(args, expected): if type(expected) is int: raise CFBSValidationError( name, From 4a75a946b21ca7a2905fab539779ede20afeb619 Mon Sep 17 00:00:00 2001 From: jakub-nt <175944085+jakub-nt@users.noreply.github.com> Date: Tue, 24 Jun 2025 16:10:09 +0200 Subject: [PATCH 5/5] Reformatted `JSON.md` to have one sentence per line Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com> --- JSON.md | 60 +++++++++++++++++++++++---------------------------------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/JSON.md b/JSON.md index 14086709..bb946f8a 100644 --- a/JSON.md +++ b/JSON.md @@ -223,7 +223,8 @@ These are copies of the module directories, where it's more "safe" to do things ## All available build steps The build steps below manipulate the temporary files in the steps directories and write results to the output policy set, in `out/masterfiles`. -Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`). In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest of the build step by a regular space. +Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`). +In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest of the build step by a regular space. - `copy ` - Copy a single file or a directory recursively. @@ -450,11 +451,9 @@ cfbs init && cfbs add https://github.com/cfengine/some-repo ## Modules with input -Some modules allow for users to add module input by responding to questions -expressed under the `"input"` attribute in `cfbs.json`. 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`. +Some modules allow for users to add module input by responding to questions expressed under the `"input"` attribute in `cfbs.json`. +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`. ### Create single file example @@ -490,10 +489,8 @@ The `"input"` attribute takes a list of input definitions as illustrated below. } ``` -From the example above, we can see that the `"input"` list contains one input -definition. By running the command `cfbs input create-single-file`, the input -definition will be copied into `./create-single-file/input.json` along with the -user responses. +From the example above, we can see that the `"input"` list contains one input definition. +By running the command `cfbs input create-single-file`, the input definition will be copied into `./create-single-file/input.json` along with the user responses. ``` $ cfbs input create-single-file @@ -511,10 +508,8 @@ $ cat ./create-single-file/input.json ] ``` -By running `cfbs build`, augments will be generated from -`./create-single-file/input.json` and added to `./out/masterfiles/def.json`. -Note that this is dependant on the `"input ./input.json def.json"` build step in -`cfbs.json`. +By running `cfbs build`, augments will be generated from `./create-single-file/input.json` and added to `./out/masterfiles/def.json`. +Note that this is dependent on the `"input ./input.json def.json"` build step in `cfbs.json`. ``` $ cfbs build @@ -538,13 +533,11 @@ $ cat ./out/masterfiles/def.json } ``` -From the example above we can see our beloved `filename`-variable along with a -class generated by the autorun dependency. Studying our variable closer, we can -see that a namespace, bundle, and a comment, were automatically assigned some -default values. I.e. `cfbs`, the module name canonified, and `Added by 'cfbs -input'` respectivy. These defaults can easily be overridden using the -`namespace`, `bundle`, and `comment` attributes in the variable definition. E.g. -the following variable definition; +From the example above we can see our beloved `filename`-variable along with a class generated by the autorun dependency. +Studying our variable closer, we can see that a namespace, bundle, and a comment, were automatically assigned some default values. +I.e. `cfbs`, the module name canonified, and `Added by 'cfbs input'` respectively. +These defaults can easily be overridden using the `namespace`, `bundle`, and `comment` attributes in the variable definition. +E.g. the following variable definition; ```json "input": [ @@ -575,9 +568,8 @@ would produce the following augment; ### Create a single file with content example -A module that creates empty files is not too impressive on its own. Let us -instead try to extend our previous example by having the module also ask for -file contents. +A module that creates empty files is not too impressive on its own. +Let us instead try to extend our previous example by having the module also ask for file contents. ```json { @@ -615,9 +607,8 @@ file contents. } ``` -As you can see from the example above, the extension would only require us to -add another variable to the input definition. Let's have a look at the results -from running `cfbs input` with our extension module. +As you can see from the example above, the extension would only require us to add another variable to the input definition. +Let's have a look at the results from running `cfbs input` with our extension module. ``` $ cfbs input create-single-file-with-content @@ -651,9 +642,9 @@ $ cat ./out/masterfiles/def.json ### Create multiple files example -Sometimes we would like a module to support taking an arbritary number of -inputs. This can be done using a variable definition of type list. Let's extend -our first example from creating a single to multiple files. +Sometimes we would like a module to support taking an arbritary number of inputs. +This can be done using a variable definition of type list. +Let's extend our first example from creating a single to multiple files. ```json { @@ -702,8 +693,7 @@ What file should this module create? /tmp/create-multiple-files-2.txt Do you want to create another file? no ``` -The _*./create-multiple-files/input.json*_ file would look similar to the -following JSON: +The _*./create-multiple-files/input.json*_ file would look similar to the following JSON: ```json [ @@ -725,8 +715,7 @@ following JSON: ] ``` -And if we build our project we can expect something similar to the following -output: +And if we build our project we can expect something similar to the following output: ``` $ cfbs build @@ -755,8 +744,7 @@ $ cat ./out/masterfiles/def.json ### Create multiple files with content example -As a final example, let's see how we can build a module that takes an arbritary -number of filename and content pairs as input. +As a final example, let's see how we can build a module that takes an arbitrary number of filename and content pairs as input. ```json {