Skip to content

Conversation

@JakeSCahill
Copy link
Contributor

@JakeSCahill JakeSCahill commented Sep 17, 2025

This pull request introduces several enhancements and new features to the property extraction and documentation tooling, focusing on improved cloud support, more robust property parsing (especially for enterprise/advanced C++ property values), and better reporting and documentation generation. The changes include a new property comparison utility, expanded template handling for cloud-aware documentation, and improved parsing and processing logic for enterprise property values.

Wiki for tech writers on how to handle autogenerated output: https://redpandadata.atlassian.net/wiki/spaces/DOC/pages/1396244485/Review+Redpanda+configuration+properties

Testing and QA

To test these changes:

  1. Check out this branch.
    git checkout DOC-348
    
  2. Use the local npm exports:
    npm link
    
  3. Export the GITHUB_TOKEN environment variable.
    export GITHUB_TOKEN=<your-token>
    
  4. Run the property-docs CLI:
    doc-tools generate property-docs --tag v25.2.4 --diff v25.1.1 --cloud-support --overrides __tests__/docs-data/property-overrides.json
    

You should see property docs autogenerated in modules/reference as well as a JSON report about what changed betwen the versions.

The most important changes are:

Cloud Support Enhancements:

  • Added support for cloud-specific property metadata extraction and tagging throughout the property extraction pipeline. This includes new CLI flags (--cloud-support), Makefile integration, and detection of cloud support in documentation templates, enabling cloud/self-managed property tagging in generated docs. [1] [2] [3] [4]
  • Documentation templates are now selected dynamically based on the presence of cloud support metadata, with improved error handling and logging if templates are missing. [1] [2]

Enterprise Value Parsing Improvements:

  • Introduced the process_enterprise_value function in property_extractor.py to robustly parse and convert complex C++ enterprise property values (including vectors, enums, and lambdas) into user-friendly JSON representations for documentation and downstream consumption. [1] [2]
  • Enhanced the tree-sitter query in parser.py to capture a wider range of C++ expression types, ensuring all relevant enterprise values are extracted for processing.

Reporting and Comparison Tools:

  • Added a new Node.js script, compare-properties.js, which compares two property JSON files and generates detailed reports of added, changed, deprecated, and removed properties, both in console and as a JSON file.

Other Improvements:

  • Updated the package version to 4.9.0 to reflect these new features.
  • Improved logging and error handling throughout the property extraction and documentation generation process, including more informative logs for template registration and cloud config status. [1] [2] [3] [4]

These changes collectively improve the maintainability, feature set, and reliability of the property extraction and documentation tooling, especially for cloud and enterprise use cases.

@netlify
Copy link

netlify bot commented Sep 17, 2025

Deploy Preview for docs-extensions-and-macros ready!

Name Link
🔨 Latest commit b498426
🔍 Latest deploy log https://app.netlify.com/projects/docs-extensions-and-macros/deploys/68d27085f89a740007c62325
😎 Deploy Preview https://deploy-preview-130--docs-extensions-and-macros.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

The changes add Redpanda Cloud support to property-doc generation and add property comparison reporting. New CLI flag --cloud-support is threaded from bin/doc-tools.js into the extractor; when enabled the build sets CLOUD_SUPPORT=1. A new tools/property-extractor/cloud_config.py fetches and parses cloud config from GitHub and exposes CloudConfig and helpers. Property extraction and transformation now normalize enterprise values (new process_enterprise_value) and can apply cloud metadata (add_cloud_support_metadata). Handlebars generation selects cloud-aware templates when cloud metadata is present. A Node compare utility (compare-properties.js) produces structured JSON reports; doc-tools invokes it to write property-changes-<old>-to-<new>.json. package.json version bumped to 4.9.0. Makefile and requirements updated to support cloud flow.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant DT as bin/doc-tools.js
  participant Make as make(...) (spawn)
  participant PE as tools/property-extractor/property_extractor.py
  participant CC as tools/property-extractor/cloud_config.py
  participant GH as GitHub API
  participant GH_TPL as Handlebars templates
  participant FS as filesystem

  User->>DT: automation generate/property-docs --old <tagA> --new <tagB> --cloud-support
  DT->>Make: spawn extractor for <tagA> with env CLOUD_SUPPORT=1
  Make->>PE: run generation (--cloud-support)
  PE->>CC: fetch_cloud_config(GITHUB_TOKEN)
  CC->>GH: GET repo contents + latest YAML
  GH-->>CC: YAML file
  CC-->>PE: CloudConfig
  PE->>PE: add_cloud_support_metadata(properties, CloudConfig)
  PE->>GH_TPL: select/register cloud templates
  PE->>FS: write generated docs (modules/reference/...)
  DT->>Make: spawn extractor for <tagB> with env CLOUD_SUPPORT=1
  Make->>PE: run generation (--cloud-support)
  PE->>CC: fetch/apply cloud config
  PE->>GH_TPL: render new docs
  PE->>FS: write generated docs (new)
  alt both example JSONs present
    DT->>FS: read examples/<tagA>-properties.json and <tagB>-properties.json
    DT->>CP as tools/property-extractor/compare-properties.js: comparePropertyFiles(old,new,tagA,tagB,modules/reference)
    CP->>FS: write property-changes-<tagA>-to-<tagB>.json
    CP-->>DT: comparison summary
  else missing inputs
    DT-->>User: log "skip comparison"
  end
  DT-->>User: Done
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested reviewers

  • paulohtb6

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The PR contains several substantive changes that fall outside DOC-348's focused scope, including a new Node.js compare-properties CLI, a large enterprise-value parsing addition (process_enterprise_value) and parser SOURCE_QUERY expansion, template enterprise-license blocks, and non-cloud doc-tools/diff behavior changes; these broaden the review surface and are not required to implement the Cloud tagging objectives. Please split unrelated work into separate PRs (compare-properties, enterprise parsing/parser changes, doc-tools refactor, and the version bump) or clearly document why they must be bundled, add focused unit/integration tests for each area, and request reviewers with the appropriate domain expertise for parser/transformers, Node tooling, and CI infra.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely identifies the primary cloud-focused change in this PR — fetching cloud configuration to derive which properties are editable or readonly in Redpanda Cloud — and maps directly to the new cloud_config and add_cloud_support_metadata work introduced in the changeset.
Linked Issues Check ✅ Passed DOC-348 The changes implement the linked objectives by adding cloud configuration fetching and normalization (fetch_cloud_config/CloudConfig), annotating properties with cloud_editable/cloud_readonly/cloud_supported/cloud_byoc_only via add_cloud_support_metadata, wiring a --cloud-support flag and CLOUD_SUPPORT env through the Makefile and CLI, and producing cloud-aware templates that emit tag::redpanda-cloud[] markers so Cloud-only includes/views can be generated from the single-source docs.
Docstring Coverage ✅ Passed Docstring coverage is 96.88% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed The pull request description accurately and directly summarizes the changes shown in the raw file summaries and aligns with the PR objectives (cloud support, enterprise value parsing, compare-properties tooling, template updates, and a package bump), and it includes concrete testing steps and examples. The description is clearly related to the changeset and not off-topic. Therefore it satisfies the lenient criteria for this check.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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)

522-551: diffDirs requires old/new temp dirs; add defaults for metrics/rpk to avoid runtime errors

Call sites pass three args. Provide sane defaults inside the helper to preserve compatibility.

Apply this diff:

 function diffDirs(kind, oldTag, newTag, oldTempDir, newTempDir) {
-  const diffDir = path.join('tmp', 'diffs', kind, `${oldTag}_to_${newTag}`);
+  const diffDir = path.join('tmp', 'diffs', kind, `${oldTag}_to_${newTag}`);
+  // Back-compat: derive defaults if not provided by caller
+  if (!oldTempDir || !newTempDir) {
+    if (kind === 'metrics') {
+      oldTempDir = path.join('autogenerated', oldTag, 'metrics');
+      newTempDir = path.join('autogenerated', newTag, 'metrics');
+    } else if (kind === 'rpk') {
+      oldTempDir = path.join('autogenerated', oldTag, 'rpk');
+      newTempDir = path.join('autogenerated', newTag, 'rpk');
+    }
+  }
 
   if (!fs.existsSync(oldTempDir)) {
     console.error(`❌ Cannot diff: missing ${oldTempDir}`);
     process.exit(1);
   }
🧹 Nitpick comments (26)
tools/property-extractor/templates/property-cloud.hbs (1)

21-23: Standardize BYOC note wording with topic template.

Property template: “available only in Redpanda Cloud BYOC deployments.”
Topic template: “only available in Redpanda Cloud BYOC deployments.”

Pick one phrasing across templates for consistency.

-NOTE: This property is available only in Redpanda Cloud BYOC deployments.
+NOTE: This property is only available in Redpanda Cloud BYOC deployments.
tools/property-extractor/compare-properties.js (3)

117-126: Support multiple deprecation reason keys.

Upstream data may use deprecation_reason or deprecated_reason. Fall back across common keys to avoid losing the reason.

-        report.deprecatedProperties.push({
-          name,
-          reason: newProp.deprecatedReason || 'Property marked as deprecated'
-        });
+        const reason =
+          newProp.deprecatedReason ??
+          newProp.deprecation_reason ??
+          newProp.deprecated_reason ??
+          newProp.reason ??
+          'Property marked as deprecated';
+        report.deprecatedProperties.push({ name, reason });

107-108: Avoid always appending ellipses to (short) descriptions.

Only add ... when truncated.

-        description: prop.description ? prop.description.substring(0, 100) + '...' : 'No description'
+        description: prop.description
+          ? (prop.description.length > 100
+              ? prop.description.substring(0, 100) + '...'
+              : prop.description)
+          : 'No description'
-          oldDescription: oldProp.description ? oldProp.description.substring(0, 50) + '...' : 'No description',
-          newDescription: newProp.description ? newProp.description.substring(0, 50) + '...' : 'No description'
+          oldDescription: oldProp.description
+            ? (oldProp.description.length > 50
+                ? oldProp.description.substring(0, 50) + '...'
+                : oldProp.description)
+            : 'No description',
+          newDescription: newProp.description
+            ? (newProp.description.length > 50
+                ? newProp.description.substring(0, 50) + '...'
+                : newProp.description)
+            : 'No description'

Also applies to: 144-146


178-226: Sort sections by property name for deterministic output.

Stable ordering improves diffability in CI logs and artifacts.

-  if (report.newProperties.length > 0) {
+  if (report.newProperties.length > 0) {
+    report.newProperties.sort((a, b) => a.name.localeCompare(b.name));
...
-  if (report.changedDefaults.length > 0) {
+  if (report.changedDefaults.length > 0) {
+    report.changedDefaults.sort((a, b) => a.name.localeCompare(b.name));
...
-  if (report.changedTypes.length > 0) {
+  if (report.changedTypes.length > 0) {
+    report.changedTypes.sort((a, b) => a.name.localeCompare(b.name));
...
-  if (report.changedDescriptions.length > 0) {
+  if (report.changedDescriptions.length > 0) {
+    report.changedDescriptions.sort((a, b) => a.name.localeCompare(b.name));
...
-  if (report.deprecatedProperties.length > 0) {
+  if (report.deprecatedProperties.length > 0) {
+    report.deprecatedProperties.sort((a, b) => a.name.localeCompare(b.name));
...
-  if (report.removedProperties.length > 0) {
+  if (report.removedProperties.length > 0) {
+    report.removedProperties.sort((a, b) => a.name.localeCompare(b.name));
tools/property-extractor/templates/topic-property-cloud.hbs (2)

22-25: Align BYOC note text with property-cloud.hbs.

Use a single phrasing across templates for polish.

-NOTE: This property is only available in Redpanda Cloud BYOC deployments.
+NOTE: This property is available only in Redpanda Cloud BYOC deployments.

15-19: Deduplicate enterprise notice via a partial.

The same block exists in multiple templates. Extract a partial (for example, partials/_enterprise-license.hbs) and include it to reduce maintenance.

{{! partials/_enterprise-license.hbs }}
{{#if is_enterprise}}
ifndef::env-cloud[]
*Enterprise license required*: `{{enterprise_value}}` (for license details, see xref:get-started:licensing/index.adoc[Redpanda Licensing])
endif::[]
{{/if}}

Then replace the inline blocks with:

{{> enterprise-license}}
tools/property-extractor/requirements.txt (1)

4-4: Pin or range-cap requests — requests 2.32.5 is published on PyPI (uploaded Aug 18, 2025).

Open-ended "requests>=2.32.5" permits future major upgrades; replace with one of:

  • tools/property-extractor/requirements.txt (line 4): requests>=2.32.5,<3.0.0
  • tools/property-extractor/requirements.txt (line 4) (preferred for reproducible tooling): requests==2.32.5
tools/property-extractor/cloud_config.py (10)

1-1: Remove non-functional shebang or make file executable.

Shebang implies CLI usage but the file isn't executable in git; it’s imported as a module. Prefer removing it.

-#!/usr/bin/env python3

33-43: Tighten ImportError raising and shorten messages.

Raise from None to avoid chaining noise; keep guidance concise.

-try:
-    import requests
-except ImportError:
-    raise ImportError("Missing required dependency 'requests': install with pip install requests")
+try:
+    import requests
+except ImportError as err:
+    raise ImportError("Missing dependency 'requests' (pip install requests).") from None
@@
-try:
-    import yaml
-except ImportError:
-    raise ImportError("Missing required dependency 'PyYAML': install with pip install pyyaml")
+try:
+    import yaml
+except ImportError as err:
+    raise ImportError("Missing dependency 'PyYAML' (pip install pyyaml).") from None

70-88: BYOC check should be order/case‑insensitive and handle non‑list inputs.

Current equality to ['byoc'] is brittle.

-    def is_byoc_only(self, property_name: str) -> bool:
+    def is_byoc_only(self, property_name: str) -> bool:
         """Check if a property is only available for BYOC clusters."""
         for config in self.customer_managed_configs:
             if config.get('name') == property_name:
-                cluster_types = config.get('cluster_types', [])
-                return cluster_types == ['byoc']
+                cluster_types = config.get('cluster_types') or []
+                if not isinstance(cluster_types, list):
+                    return False
+                return set(map(str.lower, cluster_types)) == {'byoc'}
         return False

206-227: Support .yaml files and more flexible version filenames.

Currently only “.yml” and purely numeric dots are accepted. Include “.yaml” and tolerate rc suffixes.

-            # Look for version YAML files (e.g., "25.1.yml", "25.2.yml")
-            if file_name.endswith('.yml'):
-                version_part = file_name.replace('.yml', '')
-                # Check if it looks like a version number (e.g., "25.1", "25.2.1")
-                if version_part.replace('.', '').isdigit():
-                    version_files.append((version_part, download_url))
+            # Look for version YAML files (e.g., "25.1.yml", "25.2.yaml", "25.1-rc1.yml")
+            if file_name.endswith(('.yml', '.yaml')):
+                version_part = file_name.rsplit('.', 1)[0]
+                # Strip optional "-rcN"/"rcN" suffix when evaluating numeric parts
+                sanitized = re.sub(r'(?i)-?rc\d+$', '', version_part)
+                if sanitized.replace('.', '').isdigit():
+                    version_files.append((version_part, download_url))
                     logger.debug(f"Found version file: {file_name} -> {version_part}")

239-249: Parse rc variants while preserving original string.

Sort by a sanitized tuple but keep the original for reporting/download.

-        for version_str, download_url in version_files:
+        for version_str, download_url in version_files:
             try:
-                # Parse version string into tuple of integers
-                version_tuple = tuple(int(part) for part in version_str.split('.'))
+                # Parse version string into tuple of integers, ignoring trailing rc
+                sanitized = re.sub(r'(?i)-?rc\d+$', '', version_str)
+                version_tuple = tuple(int(part) for part in sanitized.split('.'))
                 valid_versions.append((version_tuple, version_str, download_url))
                 logger.debug(f"Valid version parsed: {version_str} -> {version_tuple}")

186-196: Use logging.exception in except blocks to retain tracebacks.

These are diagnostic paths; exception() logs stack traces.

-            logger.error(error_msg)
+            logger.exception(error_msg)
@@
-            logger.error(error_msg)
+            logger.exception(error_msg)
@@
-        logger.error(error_msg)
+        logger.exception(error_msg)
@@
-        logger.error(error_msg)
+        logger.exception(error_msg)
@@
-        logger.error(error_msg)
+        logger.exception(error_msg)
@@
-        logger.error(error_msg)
+        logger.exception(error_msg)

Also applies to: 287-288, 375-376, 387-388, 404-405, 433-434


378-385: Timeout message is inconsistent with 60s second request.

Avoid implying a specific seconds value in a generic handler, or branch by request.

-        error_msg = (
-            f"Request timeout after 30 seconds: {e}\n"
+        error_msg = (
+            f"Request timed out: {e}\n"
             "GitHub API may be experiencing issues.\n"
             "To fix:\n"
             "1. Check GitHub status: https://status.github.com/\n"
             "2. Try again in a few minutes\n"
             "3. Check network connectivity"
         )

394-406: Avoid blanket Exception; handle HTTPError explicitly.

Catch requests.exceptions.HTTPError to surface unexpected HTTP statuses distinctly; keep the final catch narrower.

-    except (GitHubAuthError, NetworkError, CloudConfigParsingError):
+    except (GitHubAuthError, NetworkError, CloudConfigParsingError):
         # Re-raise our custom exceptions
         raise
-    
-    except Exception as e:
+    except requests.exceptions.HTTPError as e:
+        logger.exception("HTTP error during cloud configuration fetch.")
+        raise NetworkError(f"HTTP error: {e}") from e
+    except Exception as e:
         error_msg = (
             f"Unexpected error fetching cloud configuration: {e}\n"
@@
-        logger.error(error_msg)
-        raise CloudConfigError(error_msg)
+        logger.exception(error_msg)
+        raise CloudConfigError(error_msg) from e

502-507: Narrow exception scope when processing properties.

Limit to data issues and keep tracebacks in logs.

-        except Exception as e:
-            error_msg = f"Error processing property '{prop_name}': {e}"
-            logger.warning(error_msg)
-            errors.append(error_msg)
+        except (KeyError, TypeError, ValueError) as e:
+            logger.exception("Error processing property '%s'", prop_name)
+            errors.append(f"{prop_name}: {e}")
             continue

509-517: Fix stray f‑string.

Minor cleanup.

-    logger.info(f"Cloud metadata application completed:")
+    logger.info("Cloud metadata application completed:")
tools/property-extractor/transformers.py (1)

5-16: Use logging instead of print for import failures.

Keeps output consistent and CI-friendly.

+import logging
@@
-    except ImportError as e:
-        print(f"ERROR: Cannot import process_enterprise_value: {e}")
+    except ImportError as e:
+        logging.debug("Cannot import process_enterprise_value: %s", e)
         return None
tools/property-extractor/generate-handlebars-docs.js (3)

115-172: Graceful fallback to non‑cloud templates if cloud partials are missing

Avoid hard‑failing when cloud templates aren’t present; fall back to the standard templates to keep docs generation resilient.

Apply this diff:

 function registerPartials(hasCloudSupport = false) {
   const templatesDir = path.join(__dirname, 'templates');
   
   try {
     console.log(`📝 Registering Handlebars templates (cloud support: ${hasCloudSupport ? 'enabled' : 'disabled'})`);
     
     // Register property partial (choose cloud or regular version)
     const propertyTemplateFile = hasCloudSupport ? 'property-cloud.hbs' : 'property.hbs';
-    const propertyTemplatePath = getTemplatePath(
+    let propertyTemplatePath = getTemplatePath(
       path.join(templatesDir, propertyTemplateFile),
       'TEMPLATE_PROPERTY'
     );
     
-    if (!fs.existsSync(propertyTemplatePath)) {
-      throw new Error(`Property template not found: ${propertyTemplatePath}`);
-    }
+    if (!fs.existsSync(propertyTemplatePath)) {
+      if (hasCloudSupport) {
+        const fallback = getTemplatePath(path.join(templatesDir, 'property.hbs'), 'TEMPLATE_PROPERTY');
+        if (fs.existsSync(fallback)) {
+          console.warn(`⚠️ Property cloud template not found; falling back to 'property.hbs'`);
+          propertyTemplatePath = fallback;
+        } else {
+          throw new Error(`Property template not found: ${propertyTemplatePath}`);
+        }
+      } else {
+        throw new Error(`Property template not found: ${propertyTemplatePath}`);
+      }
+    }
     
     const propertyTemplate = fs.readFileSync(propertyTemplatePath, 'utf8');
     handlebars.registerPartial('property', propertyTemplate);
     console.log(`✅ Registered property template: ${propertyTemplateFile}`);
     
     // Register topic property partial (choose cloud or regular version)
     const topicPropertyTemplateFile = hasCloudSupport ? 'topic-property-cloud.hbs' : 'topic-property.hbs';
-    const topicPropertyTemplatePath = getTemplatePath(
+    let topicPropertyTemplatePath = getTemplatePath(
       path.join(templatesDir, topicPropertyTemplateFile),
       'TEMPLATE_TOPIC_PROPERTY'
     );
     
-    if (!fs.existsSync(topicPropertyTemplatePath)) {
-      throw new Error(`Topic property template not found: ${topicPropertyTemplatePath}`);
-    }
+    if (!fs.existsSync(topicPropertyTemplatePath)) {
+      if (hasCloudSupport) {
+        const fallback = getTemplatePath(path.join(templatesDir, 'topic-property.hbs'), 'TEMPLATE_TOPIC_PROPERTY');
+        if (fs.existsSync(fallback)) {
+          console.warn(`⚠️ Topic cloud template not found; falling back to 'topic-property.hbs'`);
+          topicPropertyTemplatePath = fallback;
+        } else {
+          throw new Error(`Topic property template not found: ${topicPropertyTemplatePath}`);
+        }
+      } else {
+        throw new Error(`Topic property template not found: ${topicPropertyTemplatePath}`);
+      }
+    }
     
     const topicPropertyTemplate = fs.readFileSync(topicPropertyTemplatePath, 'utf8');
     handlebars.registerPartial('topic-property', topicPropertyTemplate);
     console.log(`✅ Registered topic property template: ${topicPropertyTemplateFile}`);

249-256: Broaden cloud metadata detection to any cloud_ flag*

Some pipelines set cloud_editable/cloud_readonly without cloud_supported. Detect any of these to drive template selection.

Apply this diff:

 function hasCloudSupportMetadata(properties) {
-  return Object.values(properties).some(prop => 
-    Object.prototype.hasOwnProperty.call(prop, 'cloud_supported')
-  );
+  return Object.values(properties).some((prop) => {
+    if (!prop || typeof prop !== 'object') return false;
+    return ['cloud_supported', 'cloud_editable', 'cloud_readonly', 'cloud_byoc_only']
+      .some((k) => Object.prototype.hasOwnProperty.call(prop, k));
+  });
 }

266-274: Also honor env CLOUD_SUPPORT when metadata isn’t present

Allows forcing cloud templates when building old tags or running partial pipelines.

Apply this diff:

-  // Check if cloud support is enabled
-  const hasCloudSupport = hasCloudSupportMetadata(properties);
-  if (hasCloudSupport) {
-    console.log('🌤️ Cloud support metadata detected, using cloud-aware templates');
-  }
+  // Check if cloud support is enabled (metadata or env override)
+  const envCloud = String(process.env.CLOUD_SUPPORT || '').toLowerCase();
+  const cloudOverride = envCloud === '1' || envCloud === 'true';
+  const hasCloudSupport = cloudOverride || hasCloudSupportMetadata(properties);
+  if (hasCloudSupport) {
+    console.log(`🌤️ Cloud support ${cloudOverride ? 'forced by env' : 'metadata detected'}, using cloud-aware templates`);
+  }
tools/property-extractor/parser.py (2)

17-28: Clarify comment: concatenated_string ≠ “+” operator

In tree-sitter-cpp, concatenated_string covers adjacent string literals; “+” concatenation is a binary_expression. Update the comment to avoid confusion.

Apply this diff:

-#   * concatenated_string: Handles C++ string concatenation with +
+#   * concatenated_string: Handles adjacent string literal concatenation
+#   * binary_expression: Handles operator-based concatenation (e.g., "a" + "b")

32-47: Capture binary_expression to handle “+” concatenations; verify pointer_expression still flows

Add binary_expression to the query so args like "foo" + suffix are captured. The fallback (_) should still surface pointer_expression for your state machine.

Apply this diff:

         (argument_list 
             [
                 (call_expression) @argument
                 (initializer_list) @argument
                 (template_instantiation) @argument
                 (concatenated_string) @argument
+                (binary_expression) @argument
                 (string_literal) @argument
                 (raw_string_literal) @argument
                 (identifier) @argument
                 (qualified_identifier) @argument
                 (number_literal) @argument
                 (true) @argument
                 (false) @argument
                 (_) @argument
             ]
         )? @arguments

To confirm no regression in parameter extraction (especially the transition on pointer_expression), please run a quick local parse against a known field with *this as the first argument and ensure parameters[current_parameter]["params"] is populated. If helpful, I can provide a small harness.

tools/property-extractor/property_extractor.py (2)

80-91: Redundant import of logging inside try block

You already import logging at the top; drop the inner import.

Apply this diff:

 try:
     from cloud_config import fetch_cloud_config, add_cloud_support_metadata
-    # Configure cloud_config logger to suppress INFO logs by default
-    import logging
-    logging.getLogger('cloud_config').setLevel(logging.WARNING)
+    # Configure cloud_config logger to suppress INFO logs by default
+    logging.getLogger('cloud_config').setLevel(logging.WARNING)

95-190: Normalize enterprise_value types (booleans/strings) and vector items

Currently, quoted strings and booleans may remain as string literals (e.g., "true"). Convert to proper JSON types consistently.

Apply this diff:

     # FIRST: Handle std::vector initialization patterns (highest priority)
@@
-                if current_value.strip():
-                    # Clean up the value
-                    value = current_value.strip()
-                    if value.startswith('"') and value.endswith('"'):
-                        values.append(ast.literal_eval(value))
-                    else:
-                        # Handle enum values in the vector
-                        enum_match = re.match(r'[a-zA-Z0-9_:]+::([a-zA-Z0-9_]+)', value)
-                        if enum_match:
-                            values.append(enum_match.group(1))
-                        else:
-                            values.append(value)
+                if current_value.strip():
+                    # Clean up the value
+                    value = current_value.strip()
+                    if value.startswith('"') and value.endswith('"'):
+                        values.append(ast.literal_eval(value))
+                    elif value in ('true', 'false'):
+                        values.append(value == 'true')
+                    else:
+                        # Handle enum values in the vector
+                        enum_match = re.match(r'[a-zA-Z0-9_:]+::([a-zA-Z0-9_]+)', value)
+                        if enum_match:
+                            values.append(enum_match.group(1))
+                        else:
+                            values.append(value)
@@
-        if current_value.strip():
-            value = current_value.strip()
-            if value.startswith('"') and value.endswith('"'):
-                values.append(ast.literal_eval(value))
-            else:
-                # Handle enum values in the vector
-                enum_match = re.match(r'[a-zA-Z0-9_:]+::([a-zA-Z0-9_]+)', value)
-                if enum_match:
-                    values.append(enum_match.group(1))
-                else:
-                    values.append(value)
+        if current_value.strip():
+            value = current_value.strip()
+            if value.startswith('"') and value.endswith('"'):
+                values.append(ast.literal_eval(value))
+            elif value in ('true', 'false'):
+                values.append(value == 'true')
+            else:
+                # Handle enum values in the vector
+                enum_match = re.match(r'[a-zA-Z0-9_:]+::([a-zA-Z0-9_]+)', value)
+                if enum_match:
+                    values.append(enum_match.group(1))
+                else:
+                    values.append(value)
@@
-    # FOURTH: If it's already a simple value (string, boolean, etc.), return as-is
-    if enterprise_str in ("true", "false", "OIDC") or enterprise_str.startswith('"'):
-        return enterprise_str
+    # FOURTH: Normalize simple values
+    if enterprise_str in ("true", "false"):
+        return enterprise_str == "true"
+    if enterprise_str.startswith('"') and enterprise_str.endswith('"'):
+        try:
+            return ast.literal_eval(enterprise_str)
+        except Exception:
+            return enterprise_str
+    if enterprise_str == "OIDC":
+        return "OIDC"
bin/doc-tools.js (1)

819-840: Accept alternate token env vars (GH_TOKEN/REDPANDA_GITHUB_TOKEN) too

cloud_config supports multiple env names; mirror that here to reduce false negatives.

Apply this diff:

-    if (options.cloudSupport) {
+    if (options.cloudSupport) {
       console.log('🔍 Validating cloud support dependencies...');
       
-      // Check for GITHUB_TOKEN
-      if (!process.env.GITHUB_TOKEN) {
-        console.error('❌ Cloud support requires GITHUB_TOKEN environment variable');
+      // Check for any supported token env
+      const token = process.env.GITHUB_TOKEN || process.env.GH_TOKEN || process.env.REDPANDA_GITHUB_TOKEN;
+      if (!token) {
+        console.error('❌ Cloud support requires a GitHub token (GITHUB_TOKEN, GH_TOKEN, or REDPANDA_GITHUB_TOKEN)');
         console.error('   Set up GitHub token:');
         console.error('   1. Go to https://github.com/settings/tokens');
         console.error('   2. Generate token with "repo" scope');
-        console.error('   3. Set: export GITHUB_TOKEN=your_token_here');
+        console.error('   3. Set: export GITHUB_TOKEN=your_token_here');
         process.exit(1);
       }
       
       console.log('📦 Cloud support enabled - Python dependencies will be validated during execution');
       if (process.env.VIRTUAL_ENV) {
         console.log(`   Using virtual environment: ${process.env.VIRTUAL_ENV}`);
       }
       console.log('   Required packages: pyyaml, requests');
-      console.log('✅ GitHub token validated');
+      console.log('✅ GitHub token validated');
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fb12a51 and 2bf346c.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (14)
  • bin/doc-tools.js (5 hunks)
  • package.json (1 hunks)
  • tools/property-extractor/Makefile (1 hunks)
  • tools/property-extractor/cloud_config.py (1 hunks)
  • tools/property-extractor/compare-properties.js (1 hunks)
  • tools/property-extractor/generate-handlebars-docs.js (2 hunks)
  • tools/property-extractor/parser.py (1 hunks)
  • tools/property-extractor/property_extractor.py (7 hunks)
  • tools/property-extractor/requirements.txt (1 hunks)
  • tools/property-extractor/templates/property-cloud.hbs (1 hunks)
  • tools/property-extractor/templates/property.hbs (1 hunks)
  • tools/property-extractor/templates/topic-property-cloud.hbs (1 hunks)
  • tools/property-extractor/templates/topic-property.hbs (1 hunks)
  • tools/property-extractor/transformers.py (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tools/property-extractor/compare-properties.js (2)
bin/doc-tools.js (5)
  • fs (8-8)
  • name (1043-1043)
  • args (464-464)
  • args (503-503)
  • args (1254-1267)
tools/property-extractor/generate-handlebars-docs.js (6)
  • fs (3-3)
  • data (198-201)
  • data (233-237)
  • data (263-263)
  • properties (264-264)
  • args (370-370)
tools/property-extractor/property_extractor.py (1)
tools/property-extractor/cloud_config.py (2)
  • fetch_cloud_config (91-405)
  • add_cloud_support_metadata (408-552)
tools/property-extractor/transformers.py (1)
tools/property-extractor/property_extractor.py (1)
  • process_enterprise_value (95-189)
bin/doc-tools.js (1)
tools/property-extractor/compare-properties.js (3)
  • path (15-15)
  • fs (14-14)
  • args (287-287)
🪛 Ruff (0.12.2)
tools/property-extractor/cloud_config.py

1-1: Shebang is present but file is not executable

(EXE001)


37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


37-37: Avoid specifying long messages outside the exception class

(TRY003)


42-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


42-42: Avoid specifying long messages outside the exception class

(TRY003)


149-149: Abstract raise to an inner function

(TRY301)


167-167: Abstract raise to an inner function

(TRY301)


182-182: Abstract raise to an inner function

(TRY301)


194-194: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


195-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


204-204: Abstract raise to an inner function

(TRY301)


236-236: Abstract raise to an inner function

(TRY301)


260-260: Abstract raise to an inner function

(TRY301)


285-285: Use explicit conversion flag

Replace with conversion flag

(RUF010)


287-287: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


288-288: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


298-298: Abstract raise to an inner function

(TRY301)


315-315: Abstract raise to an inner function

(TRY301)


328-328: Abstract raise to an inner function

(TRY301)


355-355: f-string without any placeholders

Remove extraneous f prefix

(F541)


361-361: Consider moving this statement to an else block

(TRY300)


375-375: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


376-376: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


387-387: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


388-388: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


394-394: Do not catch blind exception: Exception

(BLE001)


404-404: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


405-405: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


430-430: Local variable all_cloud_props is assigned to but never used

Remove assignment to unused variable all_cloud_props

(F841)


431-431: Do not catch blind exception: Exception

(BLE001)


433-433: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


434-434: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


502-502: Do not catch blind exception: Exception

(BLE001)


509-509: f-string without any placeholders

Remove extraneous f prefix

(F541)

tools/property-extractor/transformers.py

12-12: Consider moving this statement to an else block

(TRY300)


449-449: Unused method argument: file_pair

(ARG002)


452-452: Unused method argument: file_pair

(ARG002)


467-467: Do not catch blind exception: Exception

(BLE001)


467-467: Local variable e is assigned to but never used

Remove assignment to unused variable e

(F841)

⏰ 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)
tools/property-extractor/templates/property.hbs (1)

13-18: Enterprise notice looks good; please verify xref and value formatting.

  • Confirm xref:get-started:licensing/index.adoc resolves in published docs.
  • Ensure enterprise_value is already sanitized for inline code (no unintended backticks).
tools/property-extractor/templates/property-cloud.hbs (2)

60-66: Good env gating for defaults in Cloud.

Showing “Default: Available in the Redpanda Cloud Console” under env-cloud while preserving formatted defaults for non-cloud is the right behavior.


1-3: Validate tag scoping strategy for Cloud includes.

tools/property-extractor/templates/property-cloud.hbs and topic-property-cloud.hbs emit the // tag::redpanda-cloud[] / // end::redpanda-cloud[] markers only when cloud_supported is true, leaving the property content untagged otherwise. I searched the repo for AsciiDoc includes using tag=redpanda-cloud and found no matches. If downstream includes are expected to filter cloud-only content, either ensure those includes use tag=redpanda-cloud, or change the templates to always wrap the cloud-specific content with the tag markers to avoid accidental leakage of non-cloud properties.

tools/property-extractor/cloud_config.py (4)

123-127: Consider 'Bearer' auth for fine‑grained tokens.

GitHub accepts both "token" and "Bearer". If you expect fine‑grained PATs, prefer Bearer; otherwise keep as is. Confirm with your org’s token policy.


350-361: Fix stray f‑string and reduce log chatter.

Minor cleanup.
[raise_nitpick_refactor]

-        logger.info(f"Cloud configuration loaded successfully:")
+        logger.info("Cloud configuration loaded successfully:")
         logger.info(f"  Version: {latest_version}")
         logger.info(f"  Editable properties: {editable_count}")
         logger.info(f"  Readonly properties: {readonly_count}")
         logger.info(f"  Total cloud properties: {total_count}")

427-435: Remove unused variable.

all_cloud_props is assigned but never used.
[raise_nitpick_refactor]

-        all_cloud_props = cloud_config.get_all_cloud_properties()

28-28: Remove unused import.

json is not used.
[raise_nitpick_refactor]

-import json
package.json (1)

3-3: Version bump looks good.

Metadata-only change; no runtime impact.

tools/property-extractor/templates/topic-property.hbs (1)

13-21: Enterprise notice block LGTM; consistent with property.hbs.

Placement after description is sensible; env-cloud guard prevents duplication in Cloud docs.

Please confirm the same wording/link is used in property.hbs for consistency.

tools/property-extractor/Makefile (1)

59-61: Cloud flag wiring LGTM.

Conditional --cloud-support integration is correct and preserves default behavior.

tools/property-extractor/transformers.py (2)

442-448: Docstring addition LGTM.

Clear delegation to centralized processor.


455-473: Avoid catching bare Exception and unused variable.

Drop the variable name or log; keep fallback behavior.
[raise_nitpick_refactor]

-            except Exception as e:
+            except Exception:
                 # Fallback to raw value if processing fails
                 property["enterprise_value"] = enterpriseValue
bin/doc-tools.js (5)

470-521: LGTM: comparison report helper

Good guardrails and clear output paths. Spawning with cwd set to property-extractor is appropriate.


811-812: LGTM: CLI flag for cloud support

Option name aligns with downstream usage and env propagation.


902-904: LGTM: build new tag to final destination

Matches the new flow where comparison reads JSONs from modules/reference/examples.


907-908: LGTM: generate comparison after both builds

Report placement and naming are clear.


847-888: CLOUD_SUPPORT env is already converted to --cloud-support

tools/property-extractor/Makefile contains $(if $(CLOUD_SUPPORT),--cloud-support,) (line 60), so the Makefile translates the env var into the CLI flag — no change required.

tools/property-extractor/property_extractor.py (2)

1385-1397: No action — Makefile already forwards --cloud-support

tools/property-extractor/Makefile invokes the extractor with $(if $(CLOUD_SUPPORT),--cloud-support,) (around line 60), so --cloud-support is passed when CLOUD_SUPPORT=1.


1281-1288: Type consistency for enterprise_value — confirm no string-typed booleans remain

File: tools/property-extractor/property_extractor.py (lines 1281-1288)

Normalization makes enterprise_value boolean/array/string. Sandbox verification failed (no properties JSON / script error). Re-run locally and confirm there are no "true"/"false" string values in enterprise_value by running:

find modules/reference/examples -type f -name '*-properties.json' -print0 \
  | xargs -0 -n1 -I{} jq -r --arg file "{}" '
      (.properties // {})
      | to_entries[]
      | select(.value.enterprise_value? and (.value.enterprise_value == "true" or .value.enterprise_value == "false"))
      | "\($file): \(.key): \(.value.enterprise_value)"
    ' {} | sed '/^$/d'

Expect no output. If any lines are printed, convert those "true"/"false" strings to booleans (or arrays) in the extractor.

coderabbitai bot added a commit that referenced this pull request Sep 17, 2025
Docstrings generation was requested by @JakeSCahill.

* #130 (comment)

The following files were modified:

* `bin/doc-tools.js`
* `tools/property-extractor/cloud_config.py`
* `tools/property-extractor/compare-properties.js`
* `tools/property-extractor/generate-handlebars-docs.js`
* `tools/property-extractor/property_extractor.py`
* `tools/property-extractor/transformers.py`
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Note

Generated docstrings for this pull request at #131

Docstrings generation was requested by @JakeSCahill.

* #130 (comment)

The following files were modified:

* `bin/doc-tools.js`
* `tools/property-extractor/cloud_config.py`
* `tools/property-extractor/compare-properties.js`
* `tools/property-extractor/generate-handlebars-docs.js`
* `tools/property-extractor/property_extractor.py`
* `tools/property-extractor/transformers.py`

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@JakeSCahill
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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)

552-594: Critical: diffDirs signature changed — callers not updated and it may delete non‑tmp directories

  • bin/doc-tools.js:566 now defines diffDirs(kind, oldTag, newTag, oldTempDir, newTempDir) but callers still call diffDirs('metrics', oldTag, newTag) at bin/doc-tools.js:634 and diffDirs('rpk', oldTag, newTag) at bin/doc-tools.js:994 — this passes undefined for the new params and will break/behave incorrectly.
  • diffDirs currently unconditionally fs.rmSync(...) the provided paths; callers pass autogenerated//... (not tmp), so this risks deleting build artifacts.
  • Fix (both required):
    • Restore backwards compatibility OR update all call sites to pass explicit directories (example: const oldDir = path.join('autogenerated', oldTag, 'metrics'); diffDirs('metrics', oldTag, newTag, oldDir, newDir);).
    • Add a safety guard before deletion: only rmSync when both resolved paths are inside repo tmp/ (e.g., tmpRoot = path.resolve('tmp') + path.sep; require path.resolve(x)+path.sep .startsWith(tmpRoot)), otherwise warn and skip cleanup.
🧹 Nitpick comments (15)
bin/doc-tools.js (3)

477-483: Spawn with shell:false to avoid double-shell quirks

Using shell:true with an explicit interpreter is brittle across platforms. Let spawnSync execute bash directly.

-const r = spawnSync('bash', [script, ...args], { stdio: 'inherit', shell: true });
+const r = spawnSync('bash', [script, ...args], { stdio: 'inherit', shell: false });

862-883: Accept GH_TOKEN as a fallback for cloud support

Property-docs already requires a token; support GH_TOKEN like cloud-regions for consistency.

-      if (!process.env.GITHUB_TOKEN) {
+      if (!(process.env.GITHUB_TOKEN || process.env.GH_TOKEN)) {
         console.error('❌ Cloud support requires GITHUB_TOKEN environment variable');
         console.error('   Set up GitHub token:');
         console.error('   1. Go to https://github.com/settings/tokens');
         console.error('   2. Generate token with "repo" scope');
         console.error('   3. Set: export GITHUB_TOKEN=your_token_here');
         process.exit(1);
       }

915-919: Unused OUTPUT_ASCIIDOC_DIR

OUTPUT_ASCIIDOC_DIR is set only in the tempDir branch but appears unused by downstream tools. Drop it or wire it through the Makefile to avoid confusion.

tools/property-extractor/transformers.py (3)

18-24: Use logging instead of print in lazy import helper

Printing from libraries pollutes stdout. Prefer logging and include module name.

-    except ImportError as e:
-        print(f"ERROR: Cannot import process_enterprise_value: {e}")
+    except ImportError as e:
+        import logging
+        logging.getLogger(__name__).warning("Cannot import process_enterprise_value: %s", e)
         return None

501-521: Guard against empty/malformed params and avoid unused exception variable

Avoid IndexError when params is empty; also remove unused variable.

-        if info['params'] is not None:
-            enterpriseValue = info['params'][0]['value']
+        if info.get('params'):
+            first = info['params'][0]
+            enterpriseValue = first.get('value') if isinstance(first, dict) else first
@@
-            } catch Exception as e:
+            } catch Exception:
                 # Fallback to raw value if processing fails
                 property["enterprise_value"] = enterpriseValue

546-556: Robust meta{...} parsing

strip("meta{ }") removes any of those characters at both ends, not the exact wrapper. Slice explicitly.

-                meta_content = param['value'].strip("meta{ }").strip()
+                raw = param['value']
+                meta_content = raw[len("meta{"):-1].strip() if raw.endswith("}") else raw[len("meta{"):].strip()
tools/property-extractor/generate-handlebars-docs.js (2)

124-181: Expose override for topic-property partial consistently

You support TEMPLATE_TOPIC_PROPERTY here but the CLI doesn’t expose a matching flag. Add a CLI/env path in the caller or document the env var.


378-397: Avoid NaN/Infinity when properties set is empty

Division by zero logs “Infinity%”. Guard the denominator.

-    const percentage = ((emptyDescriptions.length / Object.keys(properties).length) * 100).toFixed(2);
+    const denom = Math.max(1, Object.keys(properties).length);
+    const percentage = ((emptyDescriptions.length / denom) * 100).toFixed(2);
@@
-    const percentage = ((deprecatedProperties.length / Object.keys(properties).length) * 100).toFixed(2);
+    const denom2 = Math.max(1, Object.keys(properties).length);
+    const percentage = ((deprecatedProperties.length / denom2) * 100).toFixed(2);
tools/property-extractor/property_extractor.py (3)

95-189: Enterprise value: return proper JSON types (booleans) and tolerate spaced vector inits

Return True/False for boolean literals and accept optional whitespace in std::vector initializers.

-    vector_match = re.match(r'std::vector<[^>]+>\{([^}]*)\}', enterprise_str)
+    vector_match = re.match(r'std::vector<[^>]+>\s*\{\s*([^}]*)\}', enterprise_str)
@@
-    if enterprise_str in ("true", "false", "OIDC") or enterprise_str.startswith('"'):
-        return enterprise_str
+    if enterprise_str in ("true", "false"):
+        return enterprise_str == "true"
+    if enterprise_str == "OIDC" or enterprise_str.startswith('"'):
+        return enterprise_str

1418-1442: Honor CLOUD_SUPPORT=1 env if the Makefile passes it instead of a CLI flag

This avoids coupling to Makefile internals and reduces footguns.

-    if options.verbose:
+    if options.verbose:
         logging.basicConfig(level="DEBUG")
         # Also enable INFO logging for cloud_config in verbose mode
         logging.getLogger('cloud_config').setLevel(logging.INFO)
     else:
         logging.basicConfig(level="WARNING")  # Suppress INFO logs by default
 
+    # Allow env toggle for cloud support
+    if not options.cloud_support and os.environ.get("CLOUD_SUPPORT") == "1":
+        options.cloud_support = True

562-571: Use logger instead of print in override file error paths

Keep extractor stdout clean and machine-readable.

-        except Exception as e:
-            print(f"Error reading example file {file_path}: {e}")
+        except Exception as e:
+            logger.warning("Error reading example file %s: %s", file_path, e)
             return None
tools/property-extractor/cloud_config.py (4)

37-43: Chain import errors and keep messages concise

Use “raise … from e” to preserve tracebacks, per Ruff hints.

-except ImportError:
-    raise ImportError("Missing required dependency 'requests': install with pip install requests")
+except ImportError as e:
+    raise ImportError("Missing required dependency 'requests'. Install with: pip install requests") from e
@@
-except ImportError:
-    raise ImportError("Missing required dependency 'PyYAML': install with pip install pyyaml")
+except ImportError as e:
+    raise ImportError("Missing required dependency 'PyYAML'. Install with: pip install pyyaml") from e

144-181: Simplify repeated HTTP status branches and use logging.exception in JSON parse errors

Small polish: keep error chaining and richer logs.

-        try:
-            files = response.json()
-        except ValueError as e:
-            error_msg = (
-                f"Invalid JSON response from GitHub API: {e}\n"
-                "This indicates an API format change or server error.\n"
-                "Contact development team to update integration."
-            )
-            logger.error(error_msg)
-            raise CloudConfigParsingError(error_msg)
+        try:
+            files = response.json()
+        except ValueError as e:
+            logger.exception("Invalid JSON response from GitHub API")
+            raise CloudConfigParsingError("Invalid JSON response from GitHub API") from e

447-451: Remove unused variable and narrow exception

all_cloud_props isn’t used; also avoid blind except.

-        editable_props = cloud_config.get_editable_properties()
-        readonly_props = cloud_config.get_readonly_properties() 
-        all_cloud_props = cloud_config.get_all_cloud_properties()
-    except Exception as e:
+        editable_props = cloud_config.get_editable_properties()
+        readonly_props = cloud_config.get_readonly_properties()
+    except (AttributeError, TypeError) as e:
         error_msg = f"Failed to extract property sets from cloud configuration: {e}"
         logger.error(error_msg)
         raise CloudConfigError(error_msg)

378-421: Prefer logging.exception and exception chaining in network error paths

Preserve traceback to ease debugging.

-    except requests.exceptions.ConnectionError as e:
+    except requests.exceptions.ConnectionError as e:
         error_msg = (
@@
-        logger.error(error_msg)
-        raise NetworkError(error_msg)
+        logger.exception(error_msg)
+        raise NetworkError(error_msg) from e
@@
-    except requests.exceptions.Timeout as e:
+    except requests.exceptions.Timeout as e:
@@
-        logger.error(error_msg)
-        raise NetworkError(error_msg)
+        logger.exception(error_msg)
+        raise NetworkError(error_msg) from e
@@
-    except Exception as e:
+    except Exception as e:
@@
-        logger.error(error_msg)
-        raise CloudConfigError(error_msg)
+        logger.exception(error_msg)
+        raise CloudConfigError(error_msg) from e
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2bf346c and 4abf989.

📒 Files selected for processing (6)
  • bin/doc-tools.js (6 hunks)
  • tools/property-extractor/cloud_config.py (1 hunks)
  • tools/property-extractor/compare-properties.js (1 hunks)
  • tools/property-extractor/generate-handlebars-docs.js (3 hunks)
  • tools/property-extractor/property_extractor.py (9 hunks)
  • tools/property-extractor/transformers.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tools/property-extractor/compare-properties.js
🧰 Additional context used
🪛 Ruff (0.12.2)
tools/property-extractor/cloud_config.py

1-1: Shebang is present but file is not executable

(EXE001)


37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


37-37: Avoid specifying long messages outside the exception class

(TRY003)


42-42: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


42-42: Avoid specifying long messages outside the exception class

(TRY003)


164-164: Abstract raise to an inner function

(TRY301)


182-182: Abstract raise to an inner function

(TRY301)


197-197: Abstract raise to an inner function

(TRY301)


209-209: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


210-210: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


219-219: Abstract raise to an inner function

(TRY301)


251-251: Abstract raise to an inner function

(TRY301)


275-275: Abstract raise to an inner function

(TRY301)


300-300: Use explicit conversion flag

Replace with conversion flag

(RUF010)


302-302: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


303-303: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


313-313: Abstract raise to an inner function

(TRY301)


330-330: Abstract raise to an inner function

(TRY301)


343-343: Abstract raise to an inner function

(TRY301)


370-370: f-string without any placeholders

Remove extraneous f prefix

(F541)


376-376: Consider moving this statement to an else block

(TRY300)


390-390: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


391-391: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


402-402: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


403-403: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


409-409: Do not catch blind exception: Exception

(BLE001)


419-419: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


420-420: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


447-447: Local variable all_cloud_props is assigned to but never used

Remove assignment to unused variable all_cloud_props

(F841)


448-448: Do not catch blind exception: Exception

(BLE001)


450-450: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


451-451: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

(B904)


519-519: Do not catch blind exception: Exception

(BLE001)


526-526: f-string without any placeholders

Remove extraneous f prefix

(F541)

tools/property-extractor/transformers.py

20-20: Consider moving this statement to an else block

(TRY300)


27-27: Unused method argument: info

(ARG002)


27-27: Unused method argument: file_pair

(ARG002)


467-467: Unused method argument: file_pair

(ARG002)


480-480: Unused method argument: file_pair

(ARG002)


515-515: Do not catch blind exception: Exception

(BLE001)


515-515: Local variable e is assigned to but never used

Remove assignment to unused variable e

(F841)

🔇 Additional comments (1)
tools/property-extractor/cloud_config.py (1)

370-376: Fix f-string with no placeholders

Use a plain string literal.

[raise_nitpick_refactor]

-        logger.info(f"Cloud configuration loaded successfully:")
+        logger.info("Cloud configuration loaded successfully:")

@JakeSCahill JakeSCahill merged commit 24e9bc8 into main Sep 23, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants