Skip to content

Conversation

JakeSCahill
Copy link
Contributor

Recent changes in Redpanda's codebase modified how SASL mechanism defaults are defined, which broke our property extraction automation. The automation was returning internal C++ function names and type identifiers instead of user-friendly values:

  • Enterprise values showing as function names: "is_enterprise_sasl_mechanism" instead of ["GSSAPI", "OAUTHBEARER"]
  • Type extraction showing C++ internal types: "config::sasl_mechanisms_override" instead of "object"
  • Default values displaying C++ constructor syntax instead of resolved values

The property extractor now correctly generates:

{
  "sasl_mechanisms": {
    "enterprise_value": ["GSSAPI", "OAUTHBEARER"],
    "default": ["SCRAM"]
  },
  "sasl_mechanisms_overrides": {
    "enterprise_value": "Any override containing enterprise mechanisms (GSSAPI, OAUTHBEARER).",
    "items": {
      "type": "object"
    }
  }
}

…oved type extraction

- Add dynamic lookup for enterprise SASL mechanisms from source code
- Fix template type extraction with proper bracket-counting for nested templates
- Implement constexpr identifier resolution for accurate default values
- Refactor FriendlyDefaultTransformer with production-ready architecture
- Eliminate hardcoded values in favor of dynamic source code analysis
- Add comprehensive error handling and performance optimizations
- Bump package version to 4.10.1

Resolves issues where enterprise properties showed function names instead of
actual values and improves overall code quality with zero hardcoded fallbacks.
Copy link

netlify bot commented Oct 9, 2025

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

Name Link
🔨 Latest commit 83e69dc
🔍 Latest deploy log https://app.netlify.com/projects/docs-extensions-and-macros/deploys/68e7bb96e780b700084997ab
😎 Deploy Preview https://deploy-preview-137--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.

@JakeSCahill JakeSCahill requested a review from paulohtb6 October 9, 2025 11:46
Copy link
Contributor

coderabbitai bot commented Oct 9, 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

  • package.json: Version bumped from 4.10.0 to 4.10.1.
  • tools/property-extractor/property_extractor.py: Added dynamic resolution for enterprise SASL mechanisms via get_enterprise_sasl_mechanisms(), and constexpr identifier resolution via resolve_constexpr_identifier(). Integrated these into existing type/default resolution paths with fallbacks.
  • tools/property-extractor/transformers.py: Added lazy import helper get_resolve_constexpr_identifier(). Enhanced type extraction for nested templates. Extended FriendlyDefaultTransformer with identifier resolution, sstring constructor and vector initializer parsing, and std::chrono formatting, with fallbacks.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant PE as PropertyExtractor
  participant TF as FriendlyDefaultTransformer
  participant SRC as Redpanda Source Tree
  participant FS as Filesystem

  note over PE,SRC: New/Changed: constexpr & enterprise SASL resolution

  PE->>TF: parse(default_value, type)
  alt default references constexpr identifier
    TF->>TF: _get_resolver() [lazy cache]
    TF->>PE: get_resolve_constexpr_identifier()
    TF->>SRC: resolve_constexpr_identifier(identifier)
    alt identifier found
      SRC-->>TF: literal string value
      TF-->>PE: resolved value
    else not found
      TF-->>PE: fallback to original/default
    end
  else default is vector or sstring or chrono
    TF->>TF: _process_sstring_constructor / _parse_vector_contents / CHRONO
    TF-->>PE: parsed value
  end

  note over PE,FS: Enterprise SASL mechanisms resolution

  PE->>FS: locate sasl_mechanisms.h
  FS-->>PE: file contents or not found
  alt file found and parsed
    PE->>SRC: resolve_constexpr_identifier(...) for each mechanism
    SRC-->>PE: concrete strings (where available)
    PE-->>PE: set mechanisms list
  else failure
    PE-->>PE: use fallback ["GSSAPI", "OAUTHBEARER"] or descriptive override
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • paulohtb6

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly and succinctly summarizes the main purpose of the changeset by stating that dynamic lookup for enterprise SASL mechanisms is added from the source code, which aligns precisely with the PR’s core objective.
Description Check ✅ Passed The pull request description directly addresses the broken property extraction of SASL mechanisms, details the specific automation fixes and example outputs, and clearly maps to the changes introduced in the code, demonstrating clear relevance to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 82.61% which is sufficient. The required threshold is 80.00%.

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.

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

* #137 (comment)

The following files were modified:

* `tools/property-extractor/property_extractor.py`
* `tools/property-extractor/transformers.py`
Copy link
Contributor

coderabbitai bot commented Oct 9, 2025

Note

Generated docstrings for this pull request at #138

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: 2

📜 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 f3f1a6d and d4eb0c4.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • package.json (1 hunks)
  • tools/property-extractor/property_extractor.py (6 hunks)
  • tools/property-extractor/transformers.py (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/property-extractor/transformers.py (1)
tools/property-extractor/property_extractor.py (1)
  • resolve_constexpr_identifier (343-421)
🪛 Ruff (0.13.3)
tools/property-extractor/property_extractor.py

399-399: Loop control variable dirs not used within loop body

Rename unused dirs to _dirs

(B007)


922-922: Local variable original_s is assigned to but never used

Remove assignment to unused variable original_s

(F841)

tools/property-extractor/transformers.py

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

(TRY300)


43-43: Redundant exception object included in logging.exception call

(TRY401)


461-461: Unused method argument: file_pair

(ARG002)


492-492: Redundant exception object included in logging.exception call

(TRY401)


549-549: Unused method argument: file_pair

(ARG002)

⏰ 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

coderabbitai bot and others added 4 commits October 9, 2025 13:54
Docstrings generation was requested by @JakeSCahill.

* #137 (comment)

The following files were modified:

* `tools/property-extractor/property_extractor.py`
* `tools/property-extractor/transformers.py`

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@JakeSCahill JakeSCahill merged commit 4557cc1 into main Oct 9, 2025
12 of 14 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.

2 participants