DOC-1455 End to end config property automation #129
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR adds an overrides-capable property documentation pipeline across CLI, Makefile, Python, and Node layers. It expands doc-tools CLI options (overrides, templates, output-dir), reworks tools/property-extractor/Makefile to produce split outputs and run Node-based generation, adds a Handlebars generator with helpers/templates (Node), extends the Python extractor to emit enhanced JSON, resolve C++ defaults, add config_scope, and apply overrides, and introduces tests and test data for property overrides. Minor repo housekeeping: package bump and .gitignore edits. Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CLI as bin/doc-tools.js
participant Make as tools/property-extractor/Makefile
participant PyExt as property_extractor.py
participant Hbs as generate-handlebars-docs.js
participant PyDoc as generate_docs.py
participant FS as Filesystem
Dev->>CLI: property-docs --tag <tag> [--overrides file] [--template-*] [--output-dir dir]
CLI->>Make: make tag (ENV: OVERRIDES, TEMPLATE_*, OUTPUT_*)
Make->>Make: node-deps
Make->>PyExt: generate orig.json [--enhanced-output enh.json] [--overrides file]
PyExt->>PyExt: add_config_scope, resolve_type_and_default, apply_property_overrides
PyExt-->>Make: orig.json (+ enh.json if produced)
alt enhanced JSON present
Make->>Hbs: generate-handlebars-docs.js <enh.json> -> pages/, deprecated/, all_properties.txt
Hbs->>FS: write templated outputs
else fallback
Make->>PyDoc: generate_docs.py --input-file orig.json -> AsciiDoc
PyDoc->>FS: write AsciiDoc outputs
end
Make->>FS: copy outputs to FINAL_* dirs
CLI-->>Dev: outputs available in <output-dir>
sequenceDiagram
autonumber
participant PyExt as property_extractor.py
participant Src as Source Tree
participant OV as Overrides JSON
participant JSON as Enhanced JSON
PyExt->>Src: scan (resolve_cpp_function_call) for literal/default values
Src-->>PyExt: resolved literal values (if found)
PyExt->>PyExt: resolve_type_and_default(properties, definitions)
PyExt->>OV: load overrides (description/version/example/default)
PyExt->>PyExt: apply_property_overrides(baseProps, overrides)
PyExt-->>JSON: emit enhanced JSON (and original JSON)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for docs-extensions-and-macros ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/doc-tools.js (1)
287-289: Use a C++ driver that guarantees C++ stdlib headers.gcc with
-x c++can miss C++ include paths; prefer g++.Apply this diff:
- compileCmd = cppCompiler === 'gcc' ? 'gcc' : 'clang++'; + compileCmd = cppCompiler === 'gcc' ? 'g++' : 'clang++';tools/property-extractor/property_extractor.py (1)
213-214: Fix: --recursive flag is ignored (non-recursive still uses rglob).Make non-recursive branch use glob() so the CLI switch works.
- file_iter = path.rglob("*.h") if options.recursive else path.rglob("*.h") + file_iter = path.rglob("*.h") if options.recursive else path.glob("*.h")
🧹 Nitpick comments (38)
tools/property-extractor/requirements.txt (1)
3-3: Pin an upper bound for PyYAML to avoid future breaking changes.Consider constraining to the current major to prevent surprise upgrades.
Apply this diff:
-pyyaml>=6.0 +pyyaml>=6.0,<7.0package.json (1)
3-3: Add a CHANGELOG entry for 4.8.0.Document the new overrides + Handlebars generator so downstream consumers know what changed.
tools/property-extractor/helpers/or.js (1)
1-10: Nit: simplify truthiness check.Minor style tweak; Boolean is concise and idiomatic.
- return values.some(val => !!val); + return values.some(Boolean);tools/property-extractor/package.json (1)
1-15: Harden package metadata (optional).Mark private to avoid accidental publish and declare CJS/engines for predictability.
{ "name": "redpanda-property-extractor", "version": "1.0.0", "description": "Extract and document Redpanda configuration properties", - "main": "property_extractor.py", + "main": "generate-handlebars-docs.js", + "private": true, + "type": "commonjs", + "engines": { + "node": ">=18.0.0" + }, "scripts": { "docs": "node generate-handlebars-docs.js" },tools/property-extractor/helpers/join.js (1)
17-18: Preserve empty-string separators (use nullish coalescing, not logical OR).
'' || ', 'collapses an intentionally empty separator to', '. Use??to allow empty strings.- return array.join(separator || ', '); + return array.join(separator ?? ', ');tools/property-extractor/templates/deprecated-property.hbs (1)
3-7: Confirm safety of unescaped description rendering.
{{{description}}}renders raw content. If overrides are not fully trusted/sanitized, this can allow unintended HTML/AsciiDoc injection in the built site. If inputs are trusted, document that; otherwise consider escaping or sanitizing at ingestion and emitting a Handlebars.SafeString.tools/property-extractor/helpers/formatUnits.js (1)
19-26: Broaden unit inference for common names like “*_size”.Many properties end in
_sizeand are byte-sized. Consider normalizing the name and adding a conservative mapping.- if (!name) return ''; + if (!name) return ''; + name = String(name).toLowerCase(); // Extract the last part after splitting on underscores (like Python implementation) - const parts = name.split('_'); + const parts = name.split('_'); const suffix = parts[parts.length - 1]; - return suffixToUnit[suffix] || ''; + return suffixToUnit[suffix] || (suffix === 'size' ? 'bytes' : '');tools/property-extractor/transformers.py (4)
74-89: Capture one_or_many semantics explicitly.You always emit
type: "array"even forone_or_many_property<T>(which also allows a single T). Consider flagging this so templates can communicate “single value or array”.def parse(self, property, info, file_pair): @@ - property["type"] = "array" + property["type"] = "array" property["items"] = PropertyBag() property["items"]["type"] = self.type_transformer.get_type_from_declaration( info["declaration"] ) + if self.ARRAY_PATTERN_ONE_OR_MANY in info["declaration"]: + property["accepts_single_or_array"] = True
172-185: Tighten regex to avoid over-capture in nested templates.Use non-greedy capture groups and raw strings.
- if self.OPTIONAL_PATTERN in raw_type: - raw_type = re.sub(".*std::optional<(.+)>.*", "\\1", raw_type) + if self.OPTIONAL_PATTERN in raw_type: + raw_type = re.sub(r".*std::optional<(.+?)>.*", r"\1", raw_type) - if self.ARRAY_PATTERN_STD_VECTOR in raw_type: - raw_type = re.sub(".*std::vector<(.+)>.*", "\\1", raw_type) + if self.ARRAY_PATTERN_STD_VECTOR in raw_type: + raw_type = re.sub(r".*std::vector<(.+?)>.*", r"\1", raw_type) @@ - if self.ARRAY_PATTERN_ONE_OR_MANY in raw_type: - raw_type = re.sub(".*one_or_many_property<(.+)>.*", "\\1", raw_type) + if self.ARRAY_PATTERN_ONE_OR_MANY in raw_type: + raw_type = re.sub(r".*one_or_many_property<(.+?)>.*", r"\1", raw_type) raw_type = raw_type.split()[0].replace(",", "")
191-200: Remove unreachable mapping entry.After extracting inner T,
raw_typeno longer containsvector<...>, so this mapping never matches.type_mapping = [ # (regex, type) ("^u(nsigned|int)", "integer"), ("^(int|(std::)?size_t)", "integer"), ("data_directory_path", "string"), ("filesystem::path", "string"), ("(double|float)", "number"), ("string", "string"), - ("vector<[^>]+string>", "string[]"), ("std::chrono", "integer"), ]
63-63: Silence ARG002 by marking unused parameters.Minor lint cleanups; keeps signatures intact while documenting intent.
-class IsArrayTransformer: +class IsArrayTransformer: @@ - def accepts(self, info, file_pair): + def accepts(self, info, _file_pair): """ @@ - def parse(self, property, info, file_pair): + def parse(self, property, info, _file_pair): """ Transform the property to indicate it's an array type. @@ - def accepts(self, info, file_pair): + def accepts(self, _info, _file_pair): return TrueAlso applies to: 74-74, 143-145
__tests__/docs-data/examples/admin-example.adoc (1)
2-2: Consistent capitalization: use “Admin API” throughout.Minor readability fix; aligns with the title.
-This example shows how to configure the admin API endpoint with custom addressing. +This example shows how to configure the Admin API endpoints with custom addressing. @@ -You can specify multiple admin endpoints to provide redundancy and load balancing. -The admin API is used for cluster management operations. +You can specify multiple Admin API endpoints to provide redundancy and load balancing. +The Admin API is used for cluster management operations.Also applies to: 14-15
tools/property-extractor/templates/deprecated-properties.hbs (1)
3-6: Duplicate description text (attribute and body).Optional: keep only one to avoid repetition.
:description: This is an exhaustive list of all the deprecated properties. -This is an exhaustive list of all the deprecated properties. +// (Optional) Remove duplicated body sentence if not needed.tools/property-extractor/templates/property-page.hbs (1)
17-22: Optional: render group headings if available.Improves scannability when multiple groups are present.
{{#each groups}} +{{#if this.title}} +== {{this.title}} +{{/if}} {{#each this.properties}} {{> property}} {{/each}} {{/each}}__tests__/tools/property-docs-overrides.test.js (2)
26-31: Avoid execSync buffer overflows on chatty runs.Increase maxBuffer to prevent failures in CI with verbose output.
- execSync(command, { + execSync(command, { cwd: repoRoot, stdio: 'pipe', // Capture output instead of inheriting - timeout: 120000 // 2 minute timeout for slower CI environments + timeout: 120000, // 2 minute timeout for slower CI environments + maxBuffer: 10 * 1024 * 1024 });
50-58: Also assert example file content is rendered.Validates override example propagation end-to-end.
- const content = fs.readFileSync(outFile, 'utf8'); + const content = fs.readFileSync(outFile, 'utf8'); + // Example content from admin-example.adoc should be present + expect(content).toContain('Example Configuration for Admin API'); + expect(content).toContain('address: "0.0.0.0"');tools/property-extractor/helpers/formatPropertyValue.js (1)
22-26: Remove unused parameter.suffix isn’t used—drop it to avoid confusion.
bin/doc-tools.js (2)
100-117: Improve Python detection (Windows) and robustness.Include the Windows launcher and prefer g++/clang++ for C++ header checks where relevant below.
Apply this diff:
-function requirePython(minMajor = 3, minMinor = 10) { - const candidates = ['python3', 'python', 'python3.12', 'python3.11', 'python3.10']; +function requirePython(minMajor = 3, minMinor = 10) { + const candidates = ['python3', 'python', 'python3.12', 'python3.11', 'python3.10', 'py -3'];
760-827: Validate paths early (overrides/templates) and make cross-platform-friendly spawn.Fail fast if provided files don’t exist; use shell for make on Windows.
Apply this diff:
.action((options) => { @@ - const overridesPath = options.overrides; + const overridesPath = options.overrides; + if (overridesPath && !fs.existsSync(path.resolve(overridesPath))) { + console.error(`❌ Overrides file not found: ${overridesPath}`); + process.exit(1); + } @@ - const templates = { + const templates = { propertyPage: options.templatePropertyPage, property: options.templateProperty, deprecated: options.templateDeprecated, deprecatedProperty: options.templateDeprecatedProperty }; + // Validate provided template files + for (const [key, p] of Object.entries(templates)) { + if (p && !fs.existsSync(path.resolve(p))) { + console.error(`❌ Template not found for ${key}: ${p}`); + process.exit(1); + } + } @@ - const r = spawnSync('make', args, { cwd, stdio: 'inherit', env }); + const r = spawnSync('make', args, { cwd, stdio: 'inherit', env, shell: process.platform === 'win32' });tools/property-extractor/Makefile (6)
50-50: Quote OVERRIDES path to support spaces and special characters.The flag expansion should quote the path.
- $(if $(OVERRIDES),$(if $(shell [ -f "$(OVERRIDES)" ] && echo exists),--overrides $(OVERRIDES),),) + $(if $(OVERRIDES),$(if $(shell [ -f "$(OVERRIDES)" ] && echo exists),--overrides "$(OVERRIDES)",),)
52-52: Message is misleading when no overrides are used.It always prints “(with overrides)”. Make it conditional, or drop the suffix.
- @echo "✅ Enhanced JSON (with overrides) generated at $(TOOL_ROOT)/gen/$(TAG)-properties.json" + @if [ -n "$(OVERRIDES)" ] && [ -f "$(OVERRIDES)" ]; then \ + echo "✅ Enhanced JSON (with overrides) generated at $(TOOL_ROOT)/gen/$(TAG)-properties.json"; \ + else \ + echo "✅ Enhanced JSON generated at $(TOOL_ROOT)/gen/$(TAG)-properties.json"; \ + fi
95-101: Pin tree-sitter CLI for reproducibility.
npx tree-sitterpulls the latest CLI. Pin it to the grammar’s compatible version (e.g.,[email protected]) to avoid breaks.- @cd "$(TREESITTER_DIR)" && npm install --silent && $(TREE_SITTER) generate + @cd "$(TREESITTER_DIR)" && npm install --silent && npx --yes [email protected] generate
125-129: Use rsync/cp -a to preserve perms and handle empties.Safer copies; avoids failures when directories are empty and keeps timestamps/perms.
- cp -r "$(OUTPUT_ASCIIDOC_DIR)"/* "$(FINAL_OUTPUT_DIR)/"; \ + cp -a "$(OUTPUT_ASCIIDOC_DIR)/." "$(FINAL_OUTPUT_DIR)/"; \- if [ -f "$(TOOL_ROOT)/gen/$(TAG)-properties.json" ]; then \ - cp "$(TOOL_ROOT)/gen/$(TAG)-properties.json" "$(FINAL_JSON_OUTPUT_DIR)/"; \ - fi; \ + if [ -f "$(TOOL_ROOT)/gen/$(TAG)-properties.json" ]; then \ + cp -a "$(TOOL_ROOT)/gen/$(TAG)-properties.json" "$(FINAL_JSON_OUTPUT_DIR)/"; \ + fi; \Also applies to: 133-138
153-166: Large target bodies (checkmake warnings).Optional: split
buildandcheckinto smaller phony targets to satisfymaxbodylength.
27-34: Derive child dirs from a single base to prevent drift.Make
OUTPUT_ASCIIDOC_DIRandOUTPUT_JSON_DIRderive fromOUTPUT_AUTOGENERATED_DIRto keep paths coherent.-OUTPUT_ASCIIDOC_DIR ?= $(REPO_ROOT)/autogenerated/$(TAG)/properties/pages -OUTPUT_JSON_DIR ?= $(REPO_ROOT)/autogenerated/$(TAG)/properties/examples -OUTPUT_AUTOGENERATED_DIR ?= $(REPO_ROOT)/autogenerated/$(TAG)/properties +OUTPUT_AUTOGENERATED_DIR ?= $(REPO_ROOT)/autogenerated/$(TAG)/properties +OUTPUT_ASCIIDOC_DIR ?= $(OUTPUT_AUTOGENERATED_DIR)/pages +OUTPUT_JSON_DIR ?= $(OUTPUT_AUTOGENERATED_DIR)/examplestools/property-extractor/json-to-asciidoc/generate_docs.py (2)
299-357: Default formatting: consider json.dumps to reduce custom logic.Much of the object/array pretty-printing can be simplified and made safer with
json.dumps(default, separators=(',', ': ')).
539-545: Parity with Node error reports.If the Node path becomes canonical, consider deprecating Python error files or keeping parity (same filenames/locations) to avoid confusion.
Also applies to: 547-561
tools/property-extractor/generate-handlebars-docs.js (3)
95-113: Handle missing/invalid template paths gracefully.Wrap
fs.readFileSyncin try/catch and emit a clearer error if a custom path is set but unreadable.
126-137: Undefined group title/intro.
group.titleandgroup.introare unused in config; templates should handleundefinedor you can omit them.- return { - title: group.title, - intro: group.intro, - properties: filteredProperties - }; + return { properties: filteredProperties };
244-280: Error reports: consider parity with Python (empty_type, min/max mismatches).If stakeholders rely on those, add them here to avoid regressions.
I can extend
generateErrorReports()to includeempty_type.txt,max_without_min.txt, andmin_without_max.txt.tools/property-extractor/property_extractor.py (8)
74-76: Cache expensive source scans in resolve_cpp_function_call; fix unused var.Add LRU caching and rename unused dirs to _dirs to ease repeated lookups and satisfy lint.
-def resolve_cpp_function_call(function_name): +@lru_cache(maxsize=128) +def resolve_cpp_function_call(function_name): @@ - for root, dirs, files in os.walk(model_dir): + for root, _dirs, files in os.walk(model_dir):Also applies to: 172-175
162-165: Tighten exception handling and use logging.exception; replace prints with logger.Avoid blind excepts, prefer specific exceptions, and keep logging consistent.
- except Exception as e: - logger.debug(f"Error reading {full_path}: {e}") + except (OSError, UnicodeDecodeError) as e: + logger.debug(f"Error reading {full_path}: {e}") @@ - except Exception as e: - logger.debug(f"Error reading {file_path}: {e}") + except (OSError, UnicodeDecodeError) as e: + logger.debug(f"Error reading {file_path}: {e}") @@ - print(f"Warning: Example file not found: {override['example_file']}") - print(f"Searched in: {', '.join(search_paths)}") + logger.warning(f"Example file not found: {override['example_file']}. Searched in: {', '.join(search_paths)}") return None @@ - except Exception as e: - print(f"Error reading example file {file_path}: {e}") + except (OSError, UnicodeDecodeError): + logger.exception(f"Error reading example file {file_path}") return None @@ - except Exception as e: - import traceback - logger.error(f"Error formatting YAML config: {e}") - logger.debug(f"Full traceback:\n{traceback.format_exc()}") + except Exception: + logger.exception("Error formatting YAML config") return None @@ - except Exception as e: - logging.error(f"Failed to load overrides file: {e}") + except (OSError, json.JSONDecodeError): + logging.exception("Failed to load overrides file") sys.exit(1) @@ - except IOError as e: - logging.error(f"Failed to write original output file: {e}") + except OSError: + logging.exception("Failed to write original output file") sys.exit(1) @@ - except IOError as e: - logging.error(f"Failed to write enhanced output file: {e}") + except OSError: + logging.exception("Failed to write enhanced output file") sys.exit(1)Also applies to: 188-190, 439-446, 446-448, 471-475, 1244-1246, 1305-1306, 1317-1318
1215-1219: Nit: use logging.DEBUG/INFO constants, not strings.- if options.verbose: - logging.basicConfig(level="DEBUG") - else: - logging.basicConfig(level="INFO") + if options.verbose: + logging.basicConfig(level=logging.DEBUG) + else: + logging.basicConfig(level=logging.INFO)
741-747: Include derived 'name' when expanding broker endpoint defaults.Docs example shows name composed as address:port; set it when both are available.
result["address"] = nested_result.get("address") result["port"] = nested_result.get("port") + if result.get("address") is not None and result.get("port") is not None: + result["name"] = f'{result["address"]}:{result["port"]}' break
731-733: Robust boolean conversion.processed values may not be strings; guard with str(...).lower().
- nested_result[nested_prop] = processed_nested_arg.lower() == "true" + nested_result[nested_prop] = str(processed_nested_arg).lower() == "true" @@ - result[prop] = processed_arg.lower() == "true" + result[prop] = str(processed_arg).lower() == "true" @@ - nested_result[nested_prop] = processed_nested_arg.lower() == "true" + nested_result[nested_prop] = str(processed_nested_arg).lower() == "true"Also applies to: 782-783, 1070-1071
1202-1206: Clarify --overrides help text to reflect new fields.- help='Optional JSON file with property description overrides', + help='Optional JSON file with property overrides (description, version, example, default)',
484-499: Scope mapping completeness.add_config_scope only distinguishes cluster/broker; PR mentions object storage grouping. Ensure object-storage properties are sourced or extend mapping to additional files if needed.
I can add paths/types and update mapping if you confirm the expected files and desired scope labels.
Also applies to: 246-256
606-633: Hard-coded defaults in process_cpp_patterns need validation.Values like leaders_preference -> "none", log_level -> "info" are assumptions. Confirm against current Redpanda defaults to avoid drift.
If you share the authoritative defaults (or point to headers), I can wire a data-driven map or auto-resolve them similarly to function resolution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontools/property-extractor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.gitignore(1 hunks)__tests__/docs-data/examples/admin-example.adoc(1 hunks)__tests__/docs-data/property-overrides.json(1 hunks)__tests__/tools/property-docs-overrides.test.js(1 hunks)bin/doc-tools.js(3 hunks)package.json(1 hunks)tools/property-extractor/Makefile(3 hunks)tools/property-extractor/generate-handlebars-docs.js(1 hunks)tools/property-extractor/helpers/and.js(1 hunks)tools/property-extractor/helpers/eq.js(1 hunks)tools/property-extractor/helpers/formatPropertyValue.js(1 hunks)tools/property-extractor/helpers/formatUnits.js(1 hunks)tools/property-extractor/helpers/index.js(1 hunks)tools/property-extractor/helpers/join.js(1 hunks)tools/property-extractor/helpers/ne.js(1 hunks)tools/property-extractor/helpers/not.js(1 hunks)tools/property-extractor/helpers/or.js(1 hunks)tools/property-extractor/helpers/renderPropertyExample.js(1 hunks)tools/property-extractor/json-to-asciidoc/generate_docs.py(7 hunks)tools/property-extractor/package.json(1 hunks)tools/property-extractor/property_extractor.py(7 hunks)tools/property-extractor/requirements.txt(1 hunks)tools/property-extractor/templates/deprecated-properties.hbs(1 hunks)tools/property-extractor/templates/deprecated-property.hbs(1 hunks)tools/property-extractor/templates/property-page.hbs(1 hunks)tools/property-extractor/templates/property.hbs(1 hunks)tools/property-extractor/transformers.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tools/property-extractor/helpers/and.js (1)
tools/property-extractor/helpers/or.js (1)
values(8-8)
tools/property-extractor/helpers/or.js (1)
tools/property-extractor/helpers/and.js (1)
values(8-8)
tools/property-extractor/generate-handlebars-docs.js (2)
tools/property-extractor/helpers/formatPropertyValue.js (1)
handlebars(1-1)tools/property-extractor/helpers/renderPropertyExample.js (1)
handlebars(1-1)
bin/doc-tools.js (1)
tools/redpanda-connect/generate-rpcn-connector-docs.js (1)
options(212-220)
tools/property-extractor/property_extractor.py (2)
tools/property-extractor/generate-handlebars-docs.js (4)
properties(198-198)groups(126-136)output(143-143)output(179-179)tools/property-extractor/helpers/formatPropertyValue.js (1)
result(111-111)
🪛 Ruff (0.12.2)
tools/property-extractor/transformers.py
63-63: Unused method argument: file_pair
(ARG002)
74-74: Unused method argument: file_pair
(ARG002)
143-143: Unused method argument: info
(ARG002)
143-143: Unused method argument: file_pair
(ARG002)
tools/property-extractor/property_extractor.py
162-162: Do not catch blind exception: Exception
(BLE001)
172-172: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
188-188: Do not catch blind exception: Exception
(BLE001)
446-446: Do not catch blind exception: Exception
(BLE001)
471-471: Do not catch blind exception: Exception
(BLE001)
473-473: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
548-548: Do not catch blind exception: Exception
(BLE001)
897-897: Loop control variable field_name not used within loop body
(B007)
1244-1244: Do not catch blind exception: Exception
(BLE001)
1245-1245: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1305-1305: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1317-1317: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 checkmake (0.2.2)
tools/property-extractor/Makefile
[warning] 36-36: Target body for "build" exceeds allowed length of 5 (17).
(maxbodylength)
[warning] 153-153: Target body for "check" exceeds allowed length of 5 (12).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test-all (20)
- GitHub Check: test-all (18)
- GitHub Check: test-tools (18)
- GitHub Check: test-extensions (20)
- GitHub Check: test-asciidoc-extensions (20)
- GitHub Check: test-tools (20)
- GitHub Check: test-asciidoc-extensions (18)
- GitHub Check: test-extensions (18)
- GitHub Check: test-macros (20)
- GitHub Check: test-macros (18)
- GitHub Check: Redirect rules - docs-extensions-and-macros
- GitHub Check: Header rules - docs-extensions-and-macros
- GitHub Check: Pages changed - docs-extensions-and-macros
🔇 Additional comments (20)
tools/property-extractor/requirements.txt (1)
3-3: Safe YAML loading verified; noyaml.loadusage detected in tools/property-extractor.package.json (1)
3-3: LGTM: version bump to 4.8.0.tools/property-extractor/helpers/ne.js (1)
1-9: LGTM: strict inequality helper is correct and consistent with eq.tools/property-extractor/helpers/eq.js (1)
1-9: LGTM: strict equality helper is correct.tools/property-extractor/helpers/not.js (1)
1-8: LGTM: logical NOT helper is correct..gitignore (1)
12-13: LGTM: broader node_modules ignore and normalized cloud-controlplane path.tools/property-extractor/helpers/formatUnits.js (1)
15-16: Verify “bps” meaning (bits vs bytes).“bps” conventionally means bits per second; the mapping here says “bytes per second”. Please confirm the intended unit across Redpanda config names and adjust if needed.
If “bps” should be bits:
- 'bps': 'bytes per second', + 'bps': 'bits per second',__tests__/docs-data/property-overrides.json (2)
1-53: LGTM on overrides structure and fields.Descriptions, versions, and example variants (file, YAML, inline) look consistent and exercise the pipeline well.
6-7: Verified example file presence The file__tests__/docs-data/examples/admin-example.adocexists as expected.tools/property-extractor/helpers/and.js (1)
1-10: LGTM: boolean AND helper is correct and side-effect free.tools/property-extractor/helpers/index.js (1)
1-13: LGTM: helper barrel is clean and complete.tools/property-extractor/templates/property.hbs (1)
22-27: Verify policy: should node_config properties suppress “Requires restart”?If hiding restart info for node_config is intended, keep the outer check but still use
(defined needs_restart)to prevent false positives.bin/doc-tools.js (1)
237-247: Nice: explicit Node/npm checks added for the new Handlebars pipeline.Good alignment with the generator changes.
tools/property-extractor/Makefile (1)
39-43: Good guard for missing OVERRIDES.The early exit is helpful; keeps builds deterministic.
tools/property-extractor/json-to-asciidoc/generate_docs.py (2)
121-126: CLI enhancement LGTM.
--input-fileis optional and backwards-compatible.
451-462: Config-scope grouping is a solid improvement.Using
config_scopewith fallback keeps categorization accurate.tools/property-extractor/generate-handlebars-docs.js (3)
8-15: Helper registration sanity-check is good.Early exit on non-function helpers prevents subtle template failures.
289-311: CLI UX LGTM.Clear usage, file existence check, and error reporting.
56-77: Review comment incorrect:config_scopeonly supports'cluster'and'broker', there is no'cloud'scope emitted, so replacing the existing prefix-based filter would break object-storage grouping.Likely an incorrect or invalid review comment.
tools/property-extractor/property_extractor.py (1)
306-310: No references tomerge_properties_and_definitionsoutside its definition—no downstream code expects$refindirection. Safe to remove the$refrewriting.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/doc-tools.js (1)
488-496: Fix shell injection risk in diff execution.Building a shell string with user-controlled tags and passing
shell: trueis exploitable. Use argv form, capture stdout, and handle diff's exit codes (0=no diffs, 1=diffs, >1=error).Apply:
- const cmd = `diff -ru "${oldDir}" "${newDir}" > "${patch}" || true`; - const res = spawnSync(cmd, { stdio: 'inherit', shell: true }); - - if (res.error) { - console.error(`❌ diff failed: ${res.error.message}`); - process.exit(1); - } - console.log(`✅ Wrote patch: ${patch}`); + const res = spawnSync('diff', ['-ru', oldDir, newDir], { encoding: 'utf8' }); + fs.writeFileSync(patch, res.stdout || ''); + if (res.error || (res.status !== 0 && res.status !== 1)) { + const msg = res.error ? res.error.message : `exit code ${res.status}`; + console.error(`❌ diff failed: ${msg}`); + process.exit(1); + } + console.log(`✅ Wrote patch: ${patch}`);
♻️ Duplicate comments (15)
tools/property-extractor/package.json (1)
5-5: Fix "main": pointing to a .py will break Node consumers.Set main to the JS entry (or remove it) so require()/import works.
Apply:
- "main": "property_extractor.py", + "main": "generate-handlebars-docs.js",tools/property-extractor/helpers/formatPropertyValue.js (4)
6-16: Handle arrays (and plain objects) in formatter to avoid [object Object] and unquoted elements.Current formatter doesn't handle arrays and will stringify objects poorly.
function formatValue(val) { - if (typeof val === 'string') { - return `"${val}"` - } else if (typeof val === 'boolean') { - return val ? 'true' : 'false'; - } else if (val === null || val === undefined) { - return 'null'; - } else { - return String(val); - } + if (val === null || val === undefined) return 'null'; + if (Array.isArray(val)) return `[${val.map(formatValue).join(', ')}]`; + if (typeof val === 'object') return JSON.stringify(val); + if (typeof val === 'string') return `"${val}"`; + if (typeof val === 'boolean') return val ? 'true' : 'false'; + return String(val); }
68-80: Coerce "null" string to null before formatting object fields.Prevents
"null"from being quoted in output.- let processedValue = v; - if (typeof v === 'string') { - processedValue = processDefaults(v, null); - } + let processedValue = typeof v === 'string' ? processDefaults(v) : v; + if (processedValue === 'null') processedValue = null; pairs.push(`${k}: ${formatValue(processedValue)}`);
82-108: Array element formatting: use unified formatter and fix inner "null" handling.Avoids unquoted strings and ensures nested objects render consistently.
- let processedValue = v; - if (typeof v === 'string') { - processedValue = processDefaults(v, null); - } + let processedValue = typeof v === 'string' ? processDefaults(v) : v; + if (processedValue === 'null') processedValue = null; pairs.push(`${k}: ${formatValue(processedValue)}`); @@ - } else { - formattedElements.push(String(item)); - } + } else { + formattedElements.push(formatValue(item)); + }
110-116: Don’t lowercase/strip quotes; preserve case and delegate to formatter.Lowercasing corrupts enums/hostnames and quoting becomes inconsistent.
- // For other types, convert to string and apply Python-style processing - let result = String(value).replace(/'/g, '').toLowerCase(); - - // Apply C++ processing - result = processDefaults(result, null); - - return new handlebars.SafeString(result); + // Strings: process C++ patterns; others: format directly + if (typeof value === 'string') { + const processed = processDefaults(value); + const normalized = processed === 'null' ? null : processed; + return new handlebars.SafeString(formatValue(normalized)); + } + return new handlebars.SafeString(formatValue(value));tools/property-extractor/json-to-asciidoc/generate_docs.py (1)
262-269: Guard against missing name and reuse it for suffix.Avoids AttributeError when name is absent and keeps logic consistent.
- lines = [f"=== {value.get('name')}\n\n"] + prop_name = value.get("name") or key + lines = [f"=== {prop_name}\n\n"] @@ - property_suffix = value.get("name").split('_')[-1] + property_suffix = prop_name.split('_')[-1]Also applies to: 271-274
tools/property-extractor/templates/deprecated-properties.hbs (1)
16-25: Missing object storage section: deprecated object storage properties never render.Add an “Object storage properties” block mirroring broker/cluster.
{{#if clusterProperties}} == Cluster properties {{#each clusterProperties}} {{> deprecated-property}} {{/each}} {{/if}} +{{#if objectStorageProperties}} +== Object storage properties + +{{#each objectStorageProperties}} +{{> deprecated-property}} + +{{/each}} +{{/if}} + {{/if}}tools/property-extractor/templates/property.hbs (3)
22-27: Fix presence checks: “Requires restart” renders when missing; use defined helper.String-comparing to "undefined" is unreliable. Use a
definedhelper to check existence and still render falsey booleans correctly.-{{#if (ne defined_in "src/v/config/node_config.cc")}} -{{#if (ne needs_restart undefined)}} -*Requires restart:* {{#if needs_restart}}Yes{{else}}No{{/if}} - -{{/if}} -{{/if}} +{{#if (defined needs_restart)}} +*Requires restart:* {{#if needs_restart}}Yes{{else}}No{{/if}} + +{{/if}}
36-48: Don’t hide 0 bounds: treat 0 as a valid value, not “absent”.Plain
#ifandandwill drop0. Usedefinedaround numeric bounds; keep grouped rendering when both exist.-{{#if (and minimum maximum)}} +{{#if (and (defined minimum) (defined maximum))}} *Accepted values:* [`{{minimum}}`, `{{maximum}}`] {{else}} -{{#if minimum}} +{{#if (defined minimum)}} *Minimum value:* `{{minimum}}` {{/if}} -{{#if maximum}} +{{#if (defined maximum)}} *Maximum value:* `{{maximum}}` {{/if}} {{/if}}
49-52: Show default when it’s 0/false: use defined check.Avoid hiding explicit false/0 defaults.
-{{#if default}} +{{#if (defined default)}} *Default:* `{{formatPropertyValue default type}}` {{/if}}tools/property-extractor/helpers/renderPropertyExample.js (1)
15-39: Use correct fences (JSON vs YAML), detect pre-wrapped content, and wrap arrays.Currently JSON is emitted as YAML and arrays may render unfenced. Improve detection and language tags.
// Handle different example formats - if (typeof property.example === 'string') { - // Check if it's already a complete AsciiDoc example - if (property.example.includes('.Example') || property.example.includes('[,yaml]')) { - exampleContent = property.example; - } else { - // Simple string example - wrap it - exampleContent = `.Example\n[,yaml]\n----\n${property.name}: ${property.example}\n----`; - } - } else if (Array.isArray(property.example)) { - // Multiline array example - exampleContent = property.example.join('\n'); - } else if (typeof property.example === 'object' && property.example.title) { + const isPreWrapped = (str) => + str.includes('.Example') || + str.includes('[,yaml]') || + str.includes('[,json]') || + str.includes('----') || + str.includes('include::'); + + if (typeof property.example === 'string') { + // Already a complete AsciiDoc example? + if (isPreWrapped(property.example)) { + exampleContent = property.example; + } else { + // Simple string example — treat as YAML snippet under the property name + exampleContent = `.Example\n[,yaml]\n----\n${property.name}: ${property.example}\n----`; + } + } else if (Array.isArray(property.example)) { + // Multiline array example + const joined = property.example.join('\n'); + exampleContent = isPreWrapped(joined) + ? joined + : `.Example\n[,yaml]\n----\n${joined}\n----`; + } else if (typeof property.example === 'object' && property.example.title) { // Structured example with title and content exampleContent = `.${property.example.title}\n`; if (property.example.description) { exampleContent += `${property.example.description}\n\n`; } if (property.example.config) { - exampleContent += `[,yaml]\n----\n${JSON.stringify(property.example.config, null, 2)}\n----`; + if (typeof property.example.config === 'string') { + // Assume provided string is YAML + exampleContent += `[,yaml]\n----\n${property.example.config}\n----`; + } else { + // Object: emit as JSON with correct language tag + exampleContent += `[,json]\n----\n${JSON.stringify(property.example.config, null, 2)}\n----`; + } } } else { // Fallback: JSON stringify the example - exampleContent = `.Example\n[,yaml]\n----\n${property.name}: ${JSON.stringify(property.example, null, 2)}\n----`; + exampleContent = `.Example\n[,json]\n----\n${property.name}: ${JSON.stringify(property.example, null, 2)}\n----`; }tools/property-extractor/property_extractor.py (2)
312-380: Do not mutate input in apply_property_overrides; return a deep-copied object.
Prevents cross-pipeline contamination (original vs enhanced).@@ -def apply_property_overrides(properties, overrides, overrides_file_path=None): +def apply_property_overrides(properties, overrides, overrides_file_path=None): @@ - if overrides and "properties" in overrides: + props = copy.deepcopy(properties) + if overrides and "properties" in overrides: for prop, override in overrides["properties"].items(): - if prop in properties: + if prop in props: @@ - if "description" in override: - properties[prop]["description"] = override["description"] + if "description" in override: + props[prop]["description"] = override["description"] @@ - if "version" in override: - properties[prop]["version"] = override["version"] + if "version" in override: + props[prop]["version"] = override["version"] @@ - if example_content: - properties[prop]["example"] = example_content + if example_content: + props[prop]["example"] = example_content @@ - if "default" in override: - properties[prop]["default"] = override["default"] - return properties + if "default" in override: + props[prop]["default"] = override["default"] + return propsAlso add:
@@ -import yaml +import yaml +import copy
1265-1291: Use deep copies for original vs enhanced pipelines.
Shallow copies share nested dicts; transformations bleed across.@@ - original_properties = add_config_scope(properties.copy()) + original_properties = add_config_scope(copy.deepcopy(properties)) @@ - enhanced_properties = apply_property_overrides(properties, overrides, options.overrides) + enhanced_properties = apply_property_overrides(copy.deepcopy(properties), overrides, options.overrides)tools/property-extractor/Makefile (2)
113-119: Fix output dir passed to Node generator (prevents pages/pages).
Pass the base output dir; the script creates pages/ under it.- cd $(TOOL_ROOT) && \ - node generate-handlebars-docs.js "gen/$(TAG)-properties.json" "$(OUTPUT_ASCIIDOC_DIR)"; \ + cd $(TOOL_ROOT) && \ + node generate-handlebars-docs.js "gen/$(TAG)-properties.json" "$(OUTPUT_AUTOGENERATED_DIR)"; \ @@ - cd $(TOOL_ROOT) && \ - node generate-handlebars-docs.js "gen/properties-output.json" "$(OUTPUT_ASCIIDOC_DIR)"; \ + cd $(TOOL_ROOT) && \ + node generate-handlebars-docs.js "gen/properties-output.json" "$(OUTPUT_AUTOGENERATED_DIR)"; \
139-149: Copy extras from the generator’s base output; fix names and paths.
Expect all_properties.txt and error/ under OUTPUT_AUTOGENERATED_DIR.- if [ -f "$(TOOL_ROOT)/gen/all-properties.txt" ]; then \ - cp "$(TOOL_ROOT)/gen/all-properties.txt" "$(FINAL_AUTOGENERATED_DIR)/"; \ + if [ -f "$(OUTPUT_AUTOGENERATED_DIR)/all_properties.txt" ]; then \ + cp "$(OUTPUT_AUTOGENERATED_DIR)/all_properties.txt" "$(FINAL_AUTOGENERATED_DIR)/"; \ fi; \ - if [ -f "$(TOOL_ROOT)/gen/errors" ]; then \ - cp "$(TOOL_ROOT)/gen/errors" "$(FINAL_AUTOGENERATED_DIR)/"; \ + if [ -d "$(OUTPUT_AUTOGENERATED_DIR)/error" ]; then \ + mkdir -p "$(FINAL_AUTOGENERATED_DIR)/error"; \ + cp -a "$(OUTPUT_AUTOGENERATED_DIR)/error/." "$(FINAL_AUTOGENERATED_DIR)/error/"; \ fi; \
🧹 Nitpick comments (31)
package.json (1)
3-3: Version bump looks fine; consider locking supported Node versions.Given new CLI/features, add an engines field to avoid unexpected breakage on older Node versions.
"bin": { "doc-tools": "./bin/doc-tools.js" }, + "engines": { + "node": ">=18.18" + },tools/property-extractor/requirements.txt (1)
3-3: Constrain PyYAML major to avoid surprise breaks; ensure safe loader.Use an upper bound and confirm yaml.safe_load (or Loader=SafeLoader) is used in the extractor.
-pyyaml>=6.0 +pyyaml>=6.0,<7__tests__/docs-data/examples/admin-example.adoc (1)
4-5: Use [source,yaml] for proper AsciiDoc syntax highlighting.-[,yaml] +[source,yaml]tools/property-extractor/templates/deprecated-property.hbs (1)
3-7: Triple-stash renders unescaped content — confirm inputs are trusted or sanitized.Using {{{description}}} is fine if the source is trusted AsciiDoc; otherwise it can inject unintended content. Verify the pipeline only feeds vetted strings or add a sanitizing helper.
tools/property-extractor/package.json (1)
1-15: Mark package private and set engines to avoid accidental publish and runtime drift.Not required, but helpful for repo tooling.
Apply:
{ "name": "redpanda-property-extractor", "version": "1.0.0", "description": "Extract and document Redpanda configuration properties", + "private": true, "main": "generate-handlebars-docs.js", "scripts": { "docs": "node generate-handlebars-docs.js" }, + "engines": { + "node": ">=18" + }, "dependencies": { "handlebars": "^4.7.8" }, "devDependencies": {}, "keywords": ["redpanda", "configuration", "documentation"], "author": "Redpanda Data" }tools/property-extractor/helpers/formatUnits.js (1)
6-17: Hoist mapping and broaden suffix coverage; normalize case.Avoid recreating the map per call; add common time units; lower-case the suffix for robustness.
Apply:
-module.exports = function formatUnits(name) { - const suffixToUnit = { - 'ms': 'milliseconds', - 'sec': 'seconds', - 'seconds': 'seconds', - 'bytes': 'bytes', - 'buf': 'bytes', - 'partitions': 'number of partitions per topic', - 'percent': 'percent', - 'bps': 'bytes per second', - 'fraction': 'fraction' - }; +const SUFFIX_TO_UNIT = { + ms: 'milliseconds', + us: 'microseconds', + ns: 'nanoseconds', + sec: 'seconds', + s: 'seconds', + second: 'seconds', + seconds: 'seconds', + min: 'minutes', + mins: 'minutes', + minute: 'minutes', + minutes: 'minutes', + hr: 'hours', + hrs: 'hours', + hour: 'hours', + hours: 'hours', + bytes: 'bytes', + buf: 'bytes', + partitions: 'number of partitions per topic', + percent: 'percent', + bps: 'bytes per second', + fraction: 'fraction' +}; + +module.exports = function formatUnits(name) { if (!name) return ''; // Extract the last part after splitting on underscores (like Python implementation) const parts = name.split('_'); - const suffix = parts[parts.length - 1]; - - return suffixToUnit[suffix] || ''; + const suffix = parts[parts.length - 1].toLowerCase(); + return SUFFIX_TO_UNIT[suffix] || ''; };Also applies to: 19-26
tools/property-extractor/transformers.py (5)
64-73: Minor: silence Ruff for unused parameter.file_pair isn’t used here; prefix with underscore.
Apply:
- def accepts(self, info, file_pair): + def accepts(self, info, _file_pair):
75-85: Capture one_or_many semantics and silence Ruff in parse().Set a flag so templates can note single-or-array acceptance; underscore unused arg.
Apply:
- def parse(self, property, info, file_pair): + def parse(self, property, info, _file_pair): @@ - property["type"] = "array" + # If the declaration uses one_or_many_property<T>, indicate the config also accepts a single value. + if self.ARRAY_PATTERN_ONE_OR_MANY in info["declaration"]: + property["accepts_single"] = True + property["type"] = "array"
172-185: Make wrapper stripping robust to nesting by looping until stable.Handles nested wrappers without relying on ordering.
Apply:
- if self.OPTIONAL_PATTERN in raw_type: - raw_type = re.sub(".*std::optional<(.+)>.*", "\\1", raw_type) - - if self.ARRAY_PATTERN_STD_VECTOR in raw_type: - raw_type = re.sub(".*std::vector<(.+)>.*", "\\1", raw_type) - - # Handle one_or_many_property<T> - extract the inner type T - # This is essential for Redpanda's flexible configuration properties - # that can accept either single values or arrays - # Check and extract from raw_type for consistency with other type extractors - if self.ARRAY_PATTERN_ONE_OR_MANY in raw_type: - raw_type = re.sub(".*one_or_many_property<(.+)>.*", "\\1", raw_type) - raw_type = raw_type.split()[0].replace(",", "") + # Iteratively strip known wrappers to reach the core T. + prev = None + while prev != raw_type: + prev = raw_type + if self.OPTIONAL_PATTERN in raw_type: + raw_type = re.sub(r".*std::optional<(.+)>.*", r"\\1", raw_type) + if self.ARRAY_PATTERN_STD_VECTOR in raw_type: + raw_type = re.sub(r".*std::vector<(.+)>.*", r"\\1", raw_type) + if self.ARRAY_PATTERN_ONE_OR_MANY in raw_type: + raw_type = re.sub(r".*one_or_many_property<(.+)>.*", r"\\1", raw_type) + raw_type = raw_type.split()[0].replace(",", "")
191-200: Remove dead pattern: vector case isn’t reachable after inner-type extraction.Avoid confusion by dropping it; arrays are handled by IsArrayTransformer.
Apply:
type_mapping = [ # (regex, type) ("^u(nsigned|int)", "integer"), ("^(int|(std::)?size_t)", "integer"), ("data_directory_path", "string"), ("filesystem::path", "string"), ("(double|float)", "number"), ("string", "string"), ("bool", "boolean"), - ("vector<[^>]+string>", "string[]"), ("std::chrono", "integer"), ]
143-145: Minor: silence Ruff for unused parameters in accepts().Rename to underscores.
Apply:
- def accepts(self, info, file_pair): + def accepts(self, _info, _file_pair):tools/property-extractor/helpers/formatPropertyValue.js (2)
22-51: Remove unused suffix param from processDefaults and callers.It’s unused; simplifying avoids confusion and reduces call noise.
-function processDefaults(inputString, suffix) { +function processDefaults(inputString) { if (typeof inputString !== 'string') { return inputString; } @@ - processedValue = processDefaults(v, null); + processedValue = processDefaults(v); @@ - processedValue = processDefaults(v, null); + processedValue = processDefaults(v); @@ - result = processDefaults(result, null); + result = processDefaults(result);Also applies to: 75-76, 97-98, 114-114
59-59: Mark unused parameter to signal intent.Rename unused argument to underscore to quiet linters without breaking template calls.
-module.exports = function formatPropertyValue(value, type) { +module.exports = function formatPropertyValue(value, _type) {tools/property-extractor/json-to-asciidoc/generate_docs.py (1)
309-326: Minor DRY: extract format_value once for reuse.You redefine format_value in two places; define once outside the branches and reuse.
tools/property-extractor/helpers/and.js (1)
6-10: Edge-case: {{and}} with no args returns true; optionally guard it.When only the Handlebars options object is passed (e.g., a misuse like
{{and}}),valuesis empty and.every(Boolean)returns true. Consider returning false for zero args to avoid surprising truthiness.Apply:
module.exports = function and(...args) { // Remove the last argument which is the Handlebars options object const values = args.slice(0, -1); - return values.every(val => Boolean(val)); + if (values.length === 0) return false; // avoid {{and}} (options only) evaluating to true + return values.every(Boolean); };__tests__/tools/property-docs-overrides.test.js (2)
71-76: Outdated comment references Python; the generator is Node/Handlebars now.Tighten wording to reflect current implementation.
- // Test 3: Simulate applying overrides (this is what the Python script would do) + // Test 3: Simulate applying overrides (mirrors what the Node.js generator does)
86-87: Add an assertion that the admin example file referenced by overrides exists.Improves integration coverage without coupling to generator internals.
}); -}); + + it('admin override example_file path exists', () => { + const overrides = JSON.parse(fs.readFileSync(overridesFile, 'utf8')); + const exampleRel = overrides.properties?.admin?.example_file; + expect(exampleRel).toBeTruthy(); + const exampleAbs = path.join(repoRoot, '__tests__', 'docs-data', exampleRel); + expect(fs.existsSync(exampleAbs)).toBe(true); + }); +});__tests__/docs-data/property-overrides.json (2)
45-48: Use a consistent example format for append_chunk_size.
Prefer an AsciiDoc/YAML block (or example_yaml) over a bare string.Apply:
- "example": "67108864" + "example_yaml": { + "title": "Example: append_chunk_size", + "description": "Set chunk size to 64 MiB.", + "config": { + "redpanda": { + "append_chunk_size": 67108864 + } + } + }
49-51: Call out secret handling in description.
Add a short caution to discourage printing/access-key leakage.- "description": "Access key for cloud storage authentication. Used to authenticate with S3-compatible object storage services." + "description": "Access key for cloud storage authentication. Used to authenticate with S3-compatible object storage services. Treat as a secret; do not include in logs or examples."tools/property-extractor/property_extractor.py (6)
399-449: Replace prints with logging and use safe_dump for YAML.
Consistent logging and safer YAML emission.@@ - if found_path: + if found_path: file_path = found_path else: - print(f"Warning: Example file not found: {override['example_file']}") - print(f"Searched in: {', '.join(search_paths)}") + logger.warning("Example file not found: %s", override.get("example_file")) + logger.warning("Searched in: %s", ", ".join(search_paths)) return None @@ - try: + try: with open(file_path, 'r', encoding='utf-8') as f: return f.read().strip() - except Exception as e: - print(f"Error reading example file {file_path}: {e}") + except Exception: + logger.exception("Error reading example file %s", file_path) return None
450-481: Minor AsciiDoc formatting + YAML safety.
Avoid extra newline; prefer safe_dump and stable key order.@@ - if description: - lines.append(f"{description}\n") + if description: + lines.append(description) @@ - yaml_content = yaml.dump(config, default_flow_style=False, indent=2) + yaml_content = yaml.safe_dump(config, default_flow_style=False, indent=2, sort_keys=False)
162-165: Log exceptions with stack traces; avoid blind catches.
Improves diagnosability while keeping behavior.- except Exception as e: - logger.debug(f"Error reading {full_path}: {e}") + except Exception: + logger.exception("Error reading %s", full_path) continue @@ - except Exception as e: - logger.debug(f"Error reading {file_path}: {e}") + except Exception: + logger.exception("Error reading %s", file_path) continueAlso applies to: 188-190
169-176: Rename unused loop variable for clarity.- for root, dirs, files in os.walk(model_dir): + for root, _dirs, files in os.walk(model_dir):
1301-1318: Use logging.exception on write failures.- except IOError as e: - logging.error(f"Failed to write original output file: {e}") + except IOError: + logging.exception("Failed to write original output file") @@ - except IOError as e: - logging.error(f"Failed to write enhanced output file: {e}") + except IOError: + logging.exception("Failed to write enhanced output file")
485-499: Consider omitting config_scope when unknown instead of None.
Cleaner JSON and avoids consumers checking for None.- else: - prop["config_scope"] = None + else: + if "config_scope" in prop: + del prop["config_scope"]tools/property-extractor/Makefile (2)
124-129: Robust copy of AsciiDoc pages.
Handle empty dirs and dotfiles.- mkdir -p "$(FINAL_OUTPUT_DIR)"; \ - cp -r "$(OUTPUT_ASCIIDOC_DIR)"/* "$(FINAL_OUTPUT_DIR)/"; \ + mkdir -p "$(FINAL_OUTPUT_DIR)"; \ + cp -a "$(OUTPUT_ASCIIDOC_DIR)/." "$(FINAL_OUTPUT_DIR)/"; \
135-137: Copy JSON to final from the staged OUTPUT_JSON_DIR, not gen.
Avoid coupling to build dir layout.- if [ -f "$(TOOL_ROOT)/gen/$(TAG)-properties.json" ]; then \ - cp "$(TOOL_ROOT)/gen/$(TAG)-properties.json" "$(FINAL_JSON_OUTPUT_DIR)/"; \ + if [ -f "$(OUTPUT_JSON_DIR)/$(TAG)-properties.json" ]; then \ + cp "$(OUTPUT_JSON_DIR)/$(TAG)-properties.json" "$(FINAL_JSON_OUTPUT_DIR)/"; \ fi; \tools/property-extractor/generate-handlebars-docs.js (2)
83-90: Harden template path handling (validate files and warn on bad env overrides).Currently, any non-existent or directory path in env silently falls back to defaults. Validate it's a file and emit a warning when an env var is set but invalid.
Apply:
function getTemplatePath(defaultPath, envVar) { const customPath = process.env[envVar]; - if (customPath && fs.existsSync(customPath)) { - console.log(`📄 Using custom template: ${customPath}`); - return customPath; - } + if (customPath) { + if (!fs.existsSync(customPath)) { + console.warn(`⚠️ ${envVar} is set but file not found: ${customPath}. Falling back to default.`); + } else if (!fs.statSync(customPath).isFile()) { + console.warn(`⚠️ ${envVar} points to a non-file: ${customPath}. Falling back to default.`); + } else { + console.log(`📄 Using custom template: ${customPath}`); + return customPath; + } + } return defaultPath; }
196-201: Validate input shape early.Fail fast if properties is not a plain object; improves DX when upstream extraction changes.
Apply:
// Read input JSON const data = JSON.parse(fs.readFileSync(inputFile, 'utf8')); - const properties = data.properties || {}; + const properties = data.properties || {}; + if (typeof properties !== 'object' || Array.isArray(properties)) { + throw new Error('Input JSON must have a "properties" object (map of name → property).'); + }bin/doc-tools.js (2)
244-247: Enforce minimum Node.js version (18+) for generator/tooling.You already require node/npm; also gate on a modern runtime to avoid fs and ESM quirks.
Apply:
// Check for Node.js (required for Handlebars templates) requireCmd('node', 'https://nodejs.org/en/download/ or use your package manager (e.g., brew install node)'); requireCmd('npm', 'Usually installed with Node.js'); + try { + const ver = execSync('node -p "process.versions.node"', { encoding: 'utf8' }).trim(); + const [maj] = ver.split('.').map(Number); + if (maj < 18) { + fail(`Node.js v18+ is required (found ${ver}). Install a newer Node: https://nodejs.org/en/download/`); + } + } catch { + /* If parsing fails, just continue; requireCmd above already enforced presence. */ + }
769-805: Warn early on non-existent override/template paths to avoid silent fallbacks.Surface misconfigurations immediately instead of only noticing changed output later.
Apply:
- if (overrides) { - env.OVERRIDES = path.resolve(overrides); - } + if (overrides) { + const abs = path.resolve(overrides); + if (!fs.existsSync(abs)) { + console.warn(`⚠️ OVERRIDES not found: ${abs} (generator may ignore overrides)`); + } else { + env.OVERRIDES = abs; + } + } if (templates.propertyPage) { - env.TEMPLATE_PROPERTY_PAGE = path.resolve(templates.propertyPage); + const abs = path.resolve(templates.propertyPage); + if (!fs.existsSync(abs)) console.warn(`⚠️ TEMPLATE_PROPERTY_PAGE not found: ${abs} (using default)`); + else env.TEMPLATE_PROPERTY_PAGE = abs; } if (templates.property) { - env.TEMPLATE_PROPERTY = path.resolve(templates.property); + const abs = path.resolve(templates.property); + if (!fs.existsSync(abs)) console.warn(`⚠️ TEMPLATE_PROPERTY not found: ${abs} (using default)`); + else env.TEMPLATE_PROPERTY = abs; } if (templates.deprecated) { - env.TEMPLATE_DEPRECATED = path.resolve(templates.deprecated); + const abs = path.resolve(templates.deprecated); + if (!fs.existsSync(abs)) console.warn(`⚠️ TEMPLATE_DEPRECATED not found: ${abs} (using default)`); + else env.TEMPLATE_DEPRECATED = abs; } if (templates.deprecatedProperty) { - env.TEMPLATE_DEPRECATED_PROPERTY = path.resolve(templates.deprecatedProperty); + const abs = path.resolve(templates.deprecatedProperty); + if (!fs.existsSync(abs)) console.warn(`⚠️ TEMPLATE_DEPRECATED_PROPERTY not found: ${abs} (using default)`); + else env.TEMPLATE_DEPRECATED_PROPERTY = abs; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontools/property-extractor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.github/workflows/test.yaml(0 hunks).gitignore(1 hunks)__tests__/docs-data/examples/admin-example.adoc(1 hunks)__tests__/docs-data/property-overrides.json(1 hunks)__tests__/tools/property-docs-overrides.test.js(1 hunks)bin/doc-tools.js(3 hunks)package.json(1 hunks)tools/property-extractor/Makefile(3 hunks)tools/property-extractor/generate-handlebars-docs.js(1 hunks)tools/property-extractor/helpers/and.js(1 hunks)tools/property-extractor/helpers/eq.js(1 hunks)tools/property-extractor/helpers/formatPropertyValue.js(1 hunks)tools/property-extractor/helpers/formatUnits.js(1 hunks)tools/property-extractor/helpers/index.js(1 hunks)tools/property-extractor/helpers/join.js(1 hunks)tools/property-extractor/helpers/ne.js(1 hunks)tools/property-extractor/helpers/not.js(1 hunks)tools/property-extractor/helpers/or.js(1 hunks)tools/property-extractor/helpers/renderPropertyExample.js(1 hunks)tools/property-extractor/json-to-asciidoc/generate_docs.py(7 hunks)tools/property-extractor/package.json(1 hunks)tools/property-extractor/property_extractor.py(7 hunks)tools/property-extractor/requirements.txt(1 hunks)tools/property-extractor/templates/deprecated-properties.hbs(1 hunks)tools/property-extractor/templates/deprecated-property.hbs(1 hunks)tools/property-extractor/templates/property-page.hbs(1 hunks)tools/property-extractor/templates/property.hbs(1 hunks)tools/property-extractor/transformers.py(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test.yaml
🧰 Additional context used
🪛 Ruff (0.12.2)
tools/property-extractor/transformers.py
63-63: Unused method argument: file_pair
(ARG002)
74-74: Unused method argument: file_pair
(ARG002)
143-143: Unused method argument: info
(ARG002)
143-143: Unused method argument: file_pair
(ARG002)
tools/property-extractor/property_extractor.py
162-162: Do not catch blind exception: Exception
(BLE001)
172-172: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
188-188: Do not catch blind exception: Exception
(BLE001)
446-446: Do not catch blind exception: Exception
(BLE001)
471-471: Do not catch blind exception: Exception
(BLE001)
473-473: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
548-548: Do not catch blind exception: Exception
(BLE001)
897-897: Loop control variable field_name not used within loop body
(B007)
1244-1244: Do not catch blind exception: Exception
(BLE001)
1245-1245: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1305-1305: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1317-1317: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🪛 checkmake (0.2.2)
tools/property-extractor/Makefile
[warning] 36-36: Target body for "build" exceeds allowed length of 5 (17).
(maxbodylength)
[warning] 153-153: Target body for "check" exceeds allowed length of 5 (12).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - docs-extensions-and-macros
- GitHub Check: Header rules - docs-extensions-and-macros
- GitHub Check: Pages changed - docs-extensions-and-macros
🔇 Additional comments (19)
.gitignore (1)
1-1: .gitignore isn’t corrupted; update the node_modules entry
Replace the trailingnode_modulesline with/node_modules/*to align with the main branch’s ignore patterns.Likely an incorrect or invalid review comment.
tools/property-extractor/helpers/not.js (1)
6-8: LGTM.Simple and correct; Handlebars will pass an options arg as last param, which is harmlessly ignored here.
tools/property-extractor/helpers/eq.js (1)
7-9: LGTM.Strict equality is appropriate for template logic; no changes needed.
tools/property-extractor/helpers/or.js (1)
6-10: LGTM.Correctly strips Handlebars options object and handles truthiness via some().
tools/property-extractor/helpers/join.js (1)
7-18: LGTM.Solid options-object guard and sensible default separator.
tools/property-extractor/helpers/ne.js (1)
1-9: LGTM: simple, strict inequality helper is correct.Matches the typical semantics of eq/ne helpers.
tools/property-extractor/transformers.py (3)
136-142: Good consistency: shared pattern constants.Centralizing wrapper identifiers reduces drift across transformers.
147-165: Docs addition reads well.Clear explanation of supported wrappers and examples.
357-360: Good: constant for vector pattern; consistent usage below.Keeps magic strings in one place.
Also applies to: 387-393
tools/property-extractor/json-to-asciidoc/generate_docs.py (6)
121-127: CLI flag addition looks good.
236-249: Docstring enrichment: clear and helpful.
287-291: Explicit object/array type display: LGTM.
359-365: Example injection support: good addition.
413-422: Custom input path resolution: LGTM.Handles absolute/relative paths cleanly.
446-462: Grouping by config_scope with legacy fallback: LGTM.This matches the new pipeline and improves accuracy.
tools/property-extractor/templates/property-page.hbs (1)
1-22: Template structure looks solid; uses helpers/partials as intended.__tests__/docs-data/property-overrides.json (2)
3-7: LGTM: admin overrides look correct and test-friendly.
Description, version, and example_file are consistent with test paths.
31-42: LGTM: Structured AsciiDoc example array is well-formed.
Good YAML block and explanatory text for abort_index_segment_size.tools/property-extractor/property_extractor.py (1)
312-380: Confirm policy: should “default” be overridable by JSON?
Linked issue emphasized description/type; overriding defaults can drift from source truth.If defaults should be source-of-truth only, I can gate this behind a flag or drop the assignment.
…b.com/redpanda-data/docs-extensions-and-macros into end-to-end-config-property-automation
…ns-and-macros into end-to-end-config-property-automation
TLDR;
This pull request introduces significant improvements to the documentation generation process for Redpanda configuration properties. The main changes include a complete migration from the Python-based AsciiDoc generator to a new Node.js/Handlebars-based system, enhanced support for property overrides, and improved test coverage for property documentation. The Makefile and supporting scripts have been refactored to provide more flexible output directory management and better error handling.
Documentation generation system overhaul:
generate-handlebars-docs.js, which generates property documentation using Handlebars templates, grouping properties by type (broker, cluster, object storage), handling deprecated properties, and generating error reports. This script replaces the previous Python-based AsciiDoc generation.and.jsandeq.js, to support advanced template logic in documentation generation. [1] [2]Makefile and build process improvements:
tools/property-extractor/Makefileto support flexible output directory configuration, improved override file validation, and switched documentation generation to use the new Node.js script and Handlebars templates. The build process now also copies generated files to final output locations as needed. [1] [2] [3]Test and override enhancements:
property-docs-overrides.test.js) to verify that property description overrides are correctly applied in generated documentation, including version and example file references.property-overrides.json) to include new descriptions, versioning, and example YAML for several properties, further improving the accuracy and clarity of generated docs.Documentation and example updates:
admin-example.adoc) demonstrating admin API configuration, referenced in the property overrides for richer documentation.These changes collectively modernize and streamline the property documentation workflow, improve maintainability, and ensure higher-quality, more customizable output.