Skip to content

Commit e83c5b0

Browse files
authored
Fix property extraction and documentation formatting (#156)
* Fix property extraction and documentation formatting - Inherit types from cluster properties for topic properties (more reliable than heuristics) - Include 'producer' in compression.type values (valid Kafka value, now listed first) - Strip trailing commas from meta field values in parser - Render property aliases as inline code in templates Fixes cleanup.policy default, compression.type values, delete.retention.ms type, and removes trailing commas from visibility fields. * Add examples back * Add readonly and editable to templates * Clean up table * Fix tests * fix context-switcher * Add more tests and clean up templates * Fix action * Fix action * Fix action * Fix action * Fix action * Remove line separator * Fix tree-sitter-cpp git checkout failure in CI The CI was failing because the GitHub Actions cache restores the build and node_modules directories without the .git directory. The Makefile check only verified if the directory exists, not if it's a valid git repository, causing git fetch/checkout commands to fail. Changed the check to look for .git directory specifically and remove any incomplete cached directories before cloning fresh.
1 parent 749ba65 commit e83c5b0

21 files changed

+2602
-297
lines changed

.github/workflows/test.yaml

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ jobs:
4848
- '__tests__/**/*tool*.test.js'
4949
property_extractor:
5050
- 'tools/property-extractor/**/*.py'
51-
- 'tools/property-extractor/tests/**'
5251
- 'tools/property-extractor/requirements.txt'
5352
package_json:
5453
- 'package.json'
@@ -221,16 +220,46 @@ jobs:
221220
cache: 'pip'
222221
cache-dependency-path: 'tools/property-extractor/requirements.txt'
223222

223+
- name: Set up Node.js
224+
uses: actions/setup-node@v4
225+
with:
226+
node-version: 20
227+
228+
- name: Get current date
229+
id: date
230+
run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_OUTPUT
231+
232+
- name: Cache Redpanda repository
233+
uses: actions/cache@v4
234+
with:
235+
path: tools/property-extractor/tmp/redpanda
236+
key: redpanda-repo-dev-${{ steps.date.outputs.date }}
237+
restore-keys: |
238+
redpanda-repo-dev-
239+
240+
- name: Cache tree-sitter build
241+
uses: actions/cache@v4
242+
with:
243+
path: |
244+
tools/property-extractor/tree-sitter/tree-sitter-cpp/build
245+
tools/property-extractor/tree-sitter/tree-sitter-cpp/node_modules
246+
key: tree-sitter-${{ runner.os }}-${{ hashFiles('tools/property-extractor/tree-sitter/tree-sitter-cpp/package-lock.json') }}
247+
restore-keys: |
248+
tree-sitter-${{ runner.os }}-
249+
224250
- name: Install dependencies
225-
working-directory: tools/property-extractor
226251
run: |
227252
python -m pip install --upgrade pip
228-
pip install -r requirements.txt
253+
pip install -r tools/property-extractor/requirements.txt
229254
230-
- name: Run property-extractor tests
255+
- name: Generate test fixtures
231256
working-directory: tools/property-extractor
232257
run: |
233-
python -m pytest tests/ -v --tb=short
258+
make build
259+
260+
- name: Run property-extractor tests
261+
run: |
262+
python -m pytest __tests__/tools/property-extractor/ -v --tb=short
234263
235264
# Run all tests if core files changed or on schedule
236265
test-all:

__tests__/extensions/process-context-switcher.test.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ describe('process-context-switcher extension', () => {
325325
expect(pageB.asciidoc.attributes['page-context-switcher']).toBe(expectedPageB);
326326

327327
// Verify that the extension detected both pages already had context switchers
328-
expect(mockLogger.warn).toHaveBeenCalledWith(
328+
expect(mockLogger.info).toHaveBeenCalledWith(
329329
expect.stringContaining('already has context-switcher attribute')
330330
);
331331
});
@@ -382,8 +382,8 @@ describe('process-context-switcher extension', () => {
382382
// Target page should keep its original context switcher unchanged
383383
expect(targetPage.asciidoc.attributes['page-context-switcher']).toBe(originalTargetAttribute);
384384

385-
// Should warn that target already has context switcher (with the existing value)
386-
expect(mockLogger.warn).toHaveBeenCalledWith(
385+
// Should log info that target already has context switcher (with the existing value)
386+
expect(mockLogger.info).toHaveBeenCalledWith(
387387
'Target page current@ROOT:console:target.adoc already has context-switcher attribute. Skipping injection to avoid overwriting existing configuration: [{"name": "Existing", "to": "somewhere-else.adoc"}]'
388388
);
389389
});
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
# Enterprise Property Tests
2+
3+
This document describes the comprehensive test suite for enterprise property detection in `test_enterprise_properties.py`.
4+
5+
## Overview
6+
7+
The test suite validates that the `EnterpriseTransformer` correctly identifies and processes all three types of enterprise properties:
8+
9+
1. **restricted_with_sanctioned**: Properties with both Enterprise (restricted) and Community (sanctioned) values
10+
2. **restricted_only**: Properties with only Enterprise (restricted) values
11+
3. **simple**: Simple enterprise validation without specific value restrictions
12+
13+
## Test Coverage (35 tests)
14+
15+
### RestrictedWithSanctionedTest (9 tests)
16+
17+
Tests for properties that have both Enterprise and Community values. These properties should:
18+
- Default to the **Enterprise (restricted)** value
19+
- Have both `enterprise_restricted_value` and `enterprise_sanctioned_value` fields
20+
21+
**Specific tests:**
22+
- `test_exactly_two_restricted_with_sanctioned_properties`: Verifies only 2 properties are classified this way
23+
- `test_core_balancing_continuous_*`: 4 tests for core_balancing_continuous
24+
- Classification as restricted_with_sanctioned
25+
- Restricted value is `["true"]`
26+
- Sanctioned value is `["false"]`
27+
- **Default is `true` (Enterprise value)**
28+
- `test_partition_autobalancing_mode_*`: 4 tests for partition_autobalancing_mode
29+
- Classification as restricted_with_sanctioned
30+
- Restricted value is `["continuous"]`
31+
- Sanctioned values are `["off", "node_add"]`
32+
- **Default is `"continuous"` (Enterprise value)**
33+
34+
### RestrictedOnlyTest (11 tests)
35+
36+
Tests for properties that have only Enterprise values. These properties should:
37+
- Default to the **Community value** (not in restricted values)
38+
- Have `enterprise_restricted_value` but NO `enterprise_sanctioned_value`
39+
40+
**Specific tests:**
41+
- `test_restricted_only_properties_exist`: At least 3 such properties exist
42+
- `test_enable_schema_id_validation_*`: 4 tests
43+
- Classification as restricted_only
44+
- Restricted values are `["compat", "redpanda"]`
45+
- Default is `"none"` (Community value)
46+
- No sanctioned value field
47+
- `test_http_authentication_*`: 3 tests
48+
- Classification as restricted_only (not restricted_with_sanctioned)
49+
- Restricted value is `["OIDC"]`
50+
- Default is `["BASIC"]` (Community value)
51+
- `test_sasl_mechanisms_*`: 3 tests
52+
- Classification as restricted_only
53+
- Restricted values are `["GSSAPI", "OAUTHBEARER"]`
54+
- Default is `["SCRAM"]` (Community value)
55+
56+
### SimpleEnterpriseTest (3 tests)
57+
58+
Tests for simple enterprise properties without value restrictions:
59+
- `test_simple_enterprise_properties_exist`: At least 1 such property exists
60+
- `test_default_leaders_preference_classification`: Example property is classified as "simple"
61+
- `test_simple_properties_have_no_restricted_values`: No restricted/sanctioned value fields
62+
63+
### EnterpriseDefaultDescriptionTest (4 tests)
64+
65+
Tests that NO properties have the removed `enterprise_default_description` field:
66+
- `test_no_enterprise_default_description_in_restricted_with_sanctioned`
67+
- `test_no_enterprise_default_description_in_restricted_only`
68+
- `test_no_enterprise_default_description_in_simple`
69+
- `test_no_enterprise_default_description_anywhere`: Comprehensive check across all properties
70+
71+
### EnterpriseValueConsistencyTest (4 tests)
72+
73+
Tests for logical consistency between values and defaults:
74+
- `test_restricted_with_sanctioned_default_matches_restricted`: Default is in restricted values
75+
- `test_restricted_only_default_not_in_restricted`: Default is NOT in restricted values
76+
- `test_restricted_with_sanctioned_have_both_values`: Both restricted and sanctioned exist
77+
- `test_restricted_only_have_only_restricted_values`: Only restricted exists, no sanctioned
78+
79+
### RegressionTest (4 tests)
80+
81+
Tests for specific bugs that were fixed during development:
82+
83+
1. **`test_http_authentication_not_restricted_with_sanctioned`**
84+
- **Bug**: http_authentication was incorrectly classified as restricted_with_sanctioned
85+
- **Cause**: Pattern detection checked params[4] vs params[0], incorrectly matching due to validator param
86+
- **Fix**: Use property name position and check specific properties by name
87+
88+
2. **`test_only_two_restricted_with_sanctioned_not_six`**
89+
- **Bug**: 6 properties detected as restricted_with_sanctioned instead of 2
90+
- **Cause**: Generic pattern matching (params[4] != params[0]) caught too many
91+
- **Fix**: Property-name-based detection for the two specific properties
92+
93+
3. **`test_core_balancing_continuous_default_not_dict`**
94+
- **Bug**: Default was overwritten to dict like `{'needs_restart': 'needs_restart::no', ...}`
95+
- **Cause**: SimpleDefaultValuesTransformer and FriendlyDefaultTransformer overwriting Enterprise default
96+
- **Fix**: Added skip conditions in those transformers for restricted_with_sanctioned
97+
98+
4. **`test_partition_autobalancing_mode_default_not_node_add`**
99+
- **Bug**: Default was 'node_add' (Community) instead of 'continuous' (Enterprise)
100+
- **Cause**: EnterpriseTransformer wasn't setting default, downstream transformers used C++ default
101+
- **Fix**: EnterpriseTransformer explicitly sets property["default"] to restricted value
102+
103+
## Running the Tests
104+
105+
```bash
106+
# Run only enterprise property tests
107+
python -m pytest tests/test_enterprise_properties.py -v
108+
109+
# Run all tests
110+
python -m pytest tests/ -v
111+
```
112+
113+
## Expected Results
114+
115+
All tests validate against `gen/dev-properties.json`. The current expected state:
116+
117+
- **2 restricted_with_sanctioned properties**:
118+
- `core_balancing_continuous`: restricted=[true], sanctioned=[false], default=true
119+
- `partition_autobalancing_mode`: restricted=[continuous], sanctioned=[off, node_add], default=continuous
120+
121+
- **3 restricted_only properties**:
122+
- `enable_schema_id_validation`: restricted=[compat, redpanda], default=none
123+
- `http_authentication`: restricted=[OIDC], default=[BASIC]
124+
- `sasl_mechanisms`: restricted=[GSSAPI, OAUTHBEARER], default=[SCRAM]
125+
126+
- **1+ simple properties**:
127+
- `default_leaders_preference`: no restricted/sanctioned values
128+
129+
- **0 properties with `enterprise_default_description` field**
130+
131+
## Maintenance
132+
133+
When modifying enterprise property detection logic:
134+
1. Run these tests first to establish baseline
135+
2. Make your changes
136+
3. Run tests again - all should still pass
137+
4. If tests fail, either:
138+
- Fix the code to match expected behavior, OR
139+
- Update tests if requirements changed (document why)
140+
141+
## Key Implementation Details
142+
143+
The tests verify several critical aspects of the transformer pipeline:
144+
145+
1. **EnterpriseTransformer runs FIRST** in the pipeline
146+
2. **For restricted_with_sanctioned**: Default is set to restricted (Enterprise) value
147+
3. **For restricted_only**: Default comes from C++ source (Community value)
148+
4. **Downstream transformers skip restricted_with_sanctioned**: Prevents default overwriting
149+
5. **Property name position matters**: Tree-sitter always captures property name at params[1]

__tests__/tools/property-extractor/setup-and-test.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,4 @@ echo "🔄 Installing Python dependencies..."
2323
# Run tests
2424
echo "🧪 Running Python tests..."
2525
cd "$SCRIPT_DIR"
26-
"$VENV/bin/python" -m pytest test_transformers.py -v
26+
"$VENV/bin/python" -m pytest . -v

0 commit comments

Comments
 (0)