feature: Reference plugin JSON schemas #2035
Conversation
WalkthroughThe pull request introduces substantial changes to the JSON schema for the RoadRunner main configuration file. Key modifications include the correction of a grammatical error in the description field, the requirement of a "version" property, and the addition of multiple new properties that reference external schemas. The schema has been restructured to enhance modularity by replacing inline definitions with external references for various components, leading to a more maintainable configuration structure. Additionally, a new GitHub Actions workflow for JSON schema validation has been created, while an existing validation job has been removed. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2035 +/- ##
=======================================
Coverage 35.95% 35.95%
=======================================
Files 17 17
Lines 1018 1018
=======================================
Hits 366 366
Misses 621 621
Partials 31 31 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
schemas/config/3.0.schema.json (2)
5-5: Fix grammatical error in descriptionChange "should contains" to "should contain" for correct grammar.
- "description": "This is your main RoadRunner configuration file. It should contains all the plugins you want to load and their configuration.", + "description": "This is your main RoadRunner configuration file. It should contain all the plugins you want to load and their configuration.",
Line range hint
1-99: Consider adding documentation for schema distributionSince this is a significant architectural change moving from monolithic to distributed schemas, please consider:
- Adding documentation explaining the new schema structure and relationships
- Providing migration guidelines for users updating from previous versions
- Including information about schema versioning strategy
- Documenting the distinction between plugins using
#/definitions/drivervs direct schema referencesThis will help users understand and adapt to the new architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- schemas/config/3.0.schema.json (2 hunks)
🔇 Additional comments (2)
schemas/config/3.0.schema.json (2)
7-9: LGTM! Good practice requiring the version field.Making the version field required helps ensure proper schema versioning and compatibility.
Line range hint
83-99: LGTM! Well-defined Duration type.The Duration definition is comprehensive with clear pattern matching and helpful examples.
Implement new test logic for schema validation Fix actual errors in .rr.yaml template
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
schemas/config/3.0.schema.json (2)
5-5: Fix grammatical error in descriptionChange "should contains" to "should contain"
- "description": "This is your main RoadRunner configuration file. It should contains all the plugins you want to load and their configuration.", + "description": "This is your main RoadRunner configuration file. It should contain all the plugins you want to load and their configuration.",
Line range hint
1-1: Use versioned reference in schema $idThe schema's
$iduses a master branch reference which could lead to breaking changes. Consider using a specific version tag for better stability.- "$id": "https://raw.githubusercontent.com/roadrunner-server/roadrunner/refs/heads/master/schemas/config/3.0.schema.json", + "$id": "https://raw.githubusercontent.com/roadrunner-server/roadrunner/v2023.x.x/schemas/config/3.0.schema.json",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
schemas/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
- .github/workflows/schema.yaml (1 hunks)
- .github/workflows/tests.yml (0 hunks)
- .gitignore (1 hunks)
- .rr.yaml (10 hunks)
- schemas/config/3.0.schema.json (2 hunks)
- schemas/package.json (1 hunks)
- schemas/readme.md (1 hunks)
- schemas/test.js (1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/tests.yml
✅ Files skipped from review due to trivial changes (1)
- schemas/package.json
🧰 Additional context used
🪛 LanguageTool
schemas/readme.md
[uncategorized] ~7-~7: You might be missing the article “an” here.
Context: ...directory inside.** - You can validate existing config file using the following command...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🪛 Biome
schemas/test.js
[error] 11-11: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 15-15: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (9)
.gitignore (1)
27-27: LGTM! The node_modules ignore pattern is appropriate.The addition of
**/node_modulesis necessary and correctly implemented to support the new JSON schema validation workflow. The pattern will properly ignore Node.js dependencies at any directory depth.Let's verify the necessity of this change by checking for the presence of Node.js related files:
✅ Verification successful
The node_modules ignore pattern is correctly added and necessary
The verification confirms the presence of
schemas/package.json, which will generate a node_modules directory when dependencies are installed. The**/node_modulespattern in .gitignore is therefore appropriate and required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of package.json files that would generate node_modules # Test: Search for package.json files. Expect: At least one occurrence in schemas/ fd package.jsonLength of output: 38
.github/workflows/schema.yaml (1)
27-29: Verify schema validation script implementation.The workflow depends on
test.jsin the schemas directory. Please ensure this script:
- Properly validates all schema files
- Handles errors gracefully
- Provides clear validation output
✅ Verification successful
Schema validation script implementation verified successfully
The test.js script properly validates schema files with the following key features:
- Uses
@apidevtools/json-schema-ref-parserto dereference and resolve all schema references- Implements proper error handling by throwing detailed validation errors
- Uses Ajv2019 for JSON Schema validation with strict mode enabled
- Handles YAML configuration files correctly using js-yaml
- Includes proper schema ID and reference management
The script is well integrated with the necessary dependencies in package.json and correctly validates against the comprehensive RoadRunner configuration schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of the test script # Check if test.js exists and show its content echo "Checking test script existence and content:" if [ -f "schemas/test.js" ]; then cat schemas/test.js else echo "Warning: schemas/test.js not found" fi # Check for package.json and its dependencies echo -e "\nChecking package.json dependencies:" if [ -f "schemas/package.json" ]; then cat schemas/package.json | grep -A 10 '"dependencies":' else echo "Warning: schemas/package.json not found" fiLength of output: 1542
Script:
#!/bin/bash # Let's verify the schema file existence and structure echo "Checking schema file existence and content:" if [ -f "schemas/config/3.0.schema.json" ]; then cat schemas/config/3.0.schema.json else echo "Warning: schemas/config/3.0.schema.json not found" fi # Check if .rr.yaml exists in the parent directory echo -e "\nChecking .rr.yaml existence:" if [ -f ".rr.yaml" ]; then cat .rr.yaml else echo "Warning: .rr.yaml not found" fiLength of output: 68143
schemas/test.js (1)
1-4: LGTM! Appropriate dependencies imported.The imports are well-chosen for JSON schema validation with proper versioning of Ajv.
schemas/config/3.0.schema.json (1)
Line range hint
84-99: Duration definition looks goodThe Duration type definition is well-documented with clear examples and a proper regex pattern.
.rr.yaml (5)
563-581: LGTM: Enhanced service observability configuration.The addition of detailed service configuration fields (
service_name,service_version,service_namespace,service_instance_id) improves observability by providing better service identification and versioning.
1963-1966: LGTM: Well-documented buffer size configuration.The TCP read buffer size configuration is well documented with clear explanation of its purpose and performance implications. The default value is explicitly stated.
979-991: LGTM: Well-structured metrics configuration.The metrics configuration follows Prometheus best practices:
- Summary metric includes properly formatted objectives
- Histogram metric defines appropriate bucket ranges
- Both metrics include descriptive help text and type labels
605-605: Verify impact of commented custom_url.The OpenTelemetry custom URL configuration has been commented out. Ensure this doesn't affect existing telemetry integrations.
#!/bin/bash # Description: Check for any telemetry endpoint configurations in the codebase rg -i "custom.*url|telemetry.*endpoint|otel.*endpoint" --type yaml
50-50: Verify security implications of commented-out user/group settings.The user and group settings for worker processes have been commented out. While this makes the configuration more flexible, it could have security implications as the processes will now run under the default RR process user/group.
Also applies to: 60-60, 65-65
✅ Verification successful
Based on the search results, I can now generate a final response. The changelog entries and configuration documentation provide enough context about the user/group settings in RoadRunner.
User/group settings can be safely commented out
The user/group settings are optional configuration parameters that were actually added as new features in previous releases (see CHANGELOG-2023.md). When these settings are commented out:
- The worker processes will run under the default system user that started RoadRunner
- This is a common and valid configuration, especially in containerized environments
- No security implications as long as RoadRunner itself is started with appropriate permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any security-related documentation or guidelines about process permissions rg -i "user|group|permission|security" README.md docs/Length of output: 244
Script:
#!/bin/bash # Let's try to find any configuration examples or documentation files in the repository fd -e yaml -e yml -e md . | xargs rg -i "user:|group:|permission|security" -A 2 -B 2 # Also search for RoadRunner configuration documentation references rg -i "roadrunner.+config" -A 2 -B 2Length of output: 10660
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- schemas/config/3.0.schema.json (2 hunks)
🔇 Additional comments (2)
schemas/config/3.0.schema.json (2)
5-5: LGTM! Description improvementThe updated description is more precise and grammatically correct.
7-9: Breaking change: Version field is now requiredThis change makes the "version" field mandatory, which could break existing configurations that don't specify it.
Let's check for configurations that might be affected:
#!/bin/bash # Description: Find .rr.yaml files missing the version field # Expected: No .rr.yaml files without version field echo "Searching for .rr.yaml files without version field..." fd -e yaml -e yml '.+\.rr\.' -x sh -c ' if ! grep -q "^version:" "{}"; then echo "Missing version in: {}" echo "---" cat "{}" echo "---" fi '
Fix invalid metrics examples Adjust workflow file
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
schemas/test.js (1)
6-20: Add JSDoc documentation for the stripIds function.The function's purpose and parameters should be clearly documented.
/** * Recursively removes $id and $schema properties from nested objects except for the root object. * This is necessary to prevent Ajv validation errors when dealing with referenced schemas. * * @param {Object} schema - The schema object to process * @param {boolean} first - Indicates if this is the root object (true) or a nested object (false) */ function stripIds(schema, first) {🧰 Tools
🪛 Biome
[error] 11-11: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
.rr.yaml (2)
548-566: Consider adding validation rules for service configurationThe OpenTelemetry service configuration includes important identifiers. Consider adding:
- Validation rules for the service name format
- Guidelines for version number format (e.g., semver)
- Best practices for namespace naming
- Format requirements for instance IDs
This will help users provide valid configuration values.
1948-1951: Document memory implications of read buffer sizeWhile the comment explains the performance benefit of a larger buffer size, it would be helpful to also document:
- Memory consumption implications (e.g., total memory = buffer_size * num_workers)
- Recommended values for different workload types
- Warning about potential memory pressure with large values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/schema.yaml (1 hunks)
- .rr.yaml (11 hunks)
- schemas/test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/schema.yaml
🧰 Additional context used
🪛 Biome
schemas/test.js
[error] 11-11: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 12-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.rr.yaml (1)
549-567: LGTM: Comprehensive OpenTelemetry service configurationThe service identification parameters are well structured. However, consider using a more deterministic value for
service_instance_idinstead of "UUID" literal.- service_instance_id: "UUID" + service_instance_id: "${POD_NAME:-UUID}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .rr.yaml (11 hunks)
🔇 Additional comments (4)
.rr.yaml (4)
106-106: Breaking change already noted in previous reviewAlso applies to: 111-111, 152-154, 158-160, 164-166
200-209: LGTM: Well-structured temporal metrics configurationThe configuration follows best practices with clear documentation and appropriate default values.
1949-1952: Consider memory implications of increased read buffer sizeThe TCP read buffer size has been increased from the default 1MB to 10MB. While this can reduce
readsyscalls for large payloads, it will increase memory usage per TCP connection. Ensure your server has sufficient memory capacity for the expected number of concurrent connections.✅ Verification successful
Let me generate additional verification scripts to understand the TCP worker pool configuration and concurrent connection handling:
This will help us understand the connection handling capacity and worker pool settings to better assess the memory impact of the increased buffer size.
Based on the verification results, I can now generate the final response:
Increased read buffer size is reasonable given the configuration
The 10MB read buffer size is appropriate considering:
- Workers are limited to 128MB memory (
max_worker_memory: 128)- Number of workers scales with CPU cores (
num_workers: 0)- No explicit connection limits are set
- The server appears to be designed for handling large TCP payloads
The memory impact is well-constrained by the worker memory limits, making this a balanced configuration for optimizing between syscall reduction and resource usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any TCP-related performance configurations or documentation rg -A 5 'read_buf_size|buffer_size|memory' --type yamlLength of output: 2711
Script:
#!/bin/bash # Search for TCP pool and connection related configurations rg -A 10 'tcp:.*pool:|num_workers|max_connections' --type yaml # Search for any TCP-related documentation or comments rg -B 2 'tcp:' --type yamlLength of output: 7230
49-49: Security: Consider implications of commented out user/group configurationsRunning the server without explicit user/group configurations means it will run with the default RR process permissions. This could be a security concern in production environments.
Also applies to: 64-64
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.rr.yaml (1)
445-445: Consider documenting empty configurationThe memory driver has an empty configuration block. Consider adding a comment explaining that no additional configuration is required for the memory driver.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .rr.yaml (11 hunks)
🔇 Additional comments (5)
.rr.yaml (5)
190-235: Well-structured metrics configurationThe temporal metrics configuration is well-organized with comprehensive documentation and appropriate defaults for both Prometheus and Statsd drivers.
549-567: Well-structured OpenTelemetry resource configurationThe service configuration follows OpenTelemetry semantic conventions with clear documentation and appropriate defaults.
1929-1932: Well-documented TCP buffer configurationThe TCP read buffer size configuration is clearly documented with appropriate default value and explanation of its impact on performance.
591-591: Verify impact of optional custom_urlThe
custom_urlconfiguration has been made optional. Please ensure this change is documented in the changelog and upgrade guide.#!/bin/bash # Check if the change is documented rg -i "custom.?url" CHANGELOG.md docs/
49-49: Security Notice: User/Group Configuration Made OptionalThe user and group configurations have been commented out, making them optional. This could have security implications if users are not aware of the default behavior.
Also applies to: 59-59, 64-64
Reason for This PR
All schemas for plugins have been moved to their own repositories.
We now validate these schemas by pulling them in by reference with node, then running the validation test on the complete schema.
This makes updating plugins a lot easier, as you will now have to update the JSON schema at the same time, ensuring a 1:1 update of properties and features.
I have fixed some errors in the template
.rr.yamlfile in the process.Description of Changes
Removed all inline schemas for plugins.
Updated tests to run on this new model.
Fixed errors in
.rr.yamldetected by fixed schemas.Reference all repos with $ref.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
package.jsonfile has been created for schema tests, including essential dependencies.Improvements