-
Notifications
You must be signed in to change notification settings - Fork 68
✨ Add Bundle-Agnostic Configuration Validation Using JSON Schema #2316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
✨ Add Bundle-Agnostic Configuration Validation Using JSON Schema #2316
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
c547c6d to
05d349b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2316 +/- ##
==========================================
- Coverage 74.30% 74.29% -0.01%
==========================================
Files 91 92 +1
Lines 7083 7217 +134
==========================================
+ Hits 5263 5362 +99
- Misses 1405 1428 +23
- Partials 415 427 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a753a5c to
f15ea9f
Compare
f15ea9f to
e9352a6
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e9352a6 to
c2e887e
Compare
1f3451a to
65bd4ef
Compare
65bd4ef to
195d51d
Compare
d78bf4a to
920f78e
Compare
920f78e to
aaa71d0
Compare
8ac9e61 to
df17573
Compare
df17573 to
33b1369
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
33b1369 to
5dfd324
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: "handles empty installNamespace gracefully when OwnNamespace only", | ||
| supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeOwnNamespace}, | ||
| rawConfig: []byte(`{"watchNamespace": "valid-ns"}`), | ||
| installNamespace: "", | ||
| expectedWatchNamespace: ptr.To("valid-ns"), | ||
| }, |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The test name says "handles empty installNamespace gracefully when OwnNamespace only", but this scenario represents a configuration issue rather than expected behavior. When an operator only supports OwnNamespace install mode (meaning watchNamespace must equal installNamespace), having an empty installNamespace makes validation meaningless.
The current implementation (lines 169-170 in config.go) skips validation when installNamespace == "", allowing any value. While this prevents crashes, it creates a confusing edge case where invalid configurations are silently accepted.
Consider either:
- Renaming the test to clarify this is testing fallback behavior (e.g., "skips validation when installNamespace is empty")
- Or changing the validation logic to require a non-empty installNamespace when format validators are active
| // UnmarshalConfig, which does the validation. | ||
| type Config struct { | ||
| WatchNamespace *string `json:"watchNamespace"` | ||
| data map[string]any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@perdasilva we are removing to make more Generic
So, I think is important we move with it , if we want change it, before we promote Config API ( consequentely single/own ns )
| return nil | ||
| } | ||
| // Extract string value | ||
| if str, ok := val.(string); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can drop now lines 100-102, because it is covered by line 103.
| // | ||
| // If the user doesn't provide any configuration but the bundle requires some fields | ||
| // (like watchNamespace), validation will fail with a helpful error. | ||
| func UnmarshalConfig(bytes []byte, schema map[string]any, installNamespace string) (*Config, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function unmarshal + validatates optionally the config. I would split these into two functions at least so that we can achieve something like:
c := NewConfigBuilder.FromBytes(bytes).WithSchema(schema).WithInstallNamespace(installNamespace).Build()
err := v.Validate()| // The Test_ErrorFormatting_* tests make sure this parsing works. If we upgrade the | ||
| // JSON schema library and those tests start failing, we'll know we need to update | ||
| // this parsing logic. | ||
| func formatSchemaError(err error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatSchemaError is unprecise because it converts ValueError into another error instance. Also, why a user could not understand the original ValueError message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minimal option here would be like
// formatSchemaError removes technical schema file paths from validation errors
// while preserving the library's clear error messages.
func formatSchemaError(err error) error {
msg := err.Error()
// Strip the "jsonschema validation failed with 'file:///.../schema.json#'" prefix
// Keep everything after the newline which has the actual error details
if idx := strings.Index(msg, "\n"); idx != -1 {
return fmt.Errorf("invalid configuration:%s", msg[idx:])
}
return fmt.Errorf("invalid configuration: %s", msg)
}However, I followed your suggestion to use DetailedOutput() / BasicOutput(), and I think we've achieved a better result with improved error messages that are neither overly fragile nor difficult to maintain.
Output Comparison (Minimal vs Current Approach)
| Scenario | Example Input | Minimal Approach | Current Approach | Winner |
|---|---|---|---|---|
| Missing required field | {} when watchNamespace required |
invalid configuration:- at '': missing property 'watchNamespace' |
required field "watchNamespace" is missing |
Current - Clearer terminology |
| Null instead of required | {"watchNamespace": null} |
invalid configuration:- at '/watchNamespace': got null, want string |
required field "watchNamespace" is missing |
Current - Recognizes intent |
| Unknown field | {"unknownField": "value"} |
invalid configuration:- at '': additional properties 'unknownField' not allowed |
unknown field "unknownField" |
Current - Concise |
| Type mismatch | {"watchNamespace": 123} |
invalid configuration:- at '/watchNamespace': got number, want string |
invalid type for field "watchNamespace": got number, want string |
Current - Adds field context |
| Nested field error | {"resources": {"memory": 512}} |
invalid configuration:- at '/resources/memory': got number, want string |
invalid type for field "resources.memory": got number, want string |
Current - Dot notation for paths |
| Root type error | true (not an object) |
invalid configuration:- at '': got boolean, want object |
invalid type: got boolean, want object |
Similar |
| Constraint violation | {"replicaCount": 0} with min=1 |
invalid configuration:- at '/replicaCount': value should be >= 1 |
value should be >= 1 |
Similar - Pass through |
| Enum violation | {"type": "Invalid"} |
invalid configuration:- at '/type': value should be one of [...] |
value should be one of [...] |
Similar - Pass through |
| Custom format validator | {"watchNamespace": "wrong-ns"} |
invalid configuration:- at '/watchNamespace': 'wrong-ns' is not valid ownNamespaceInstallMode: ... |
invalid value "wrong-ns": watchNamespace must be "install-ns" (the namespace where the operator is installed) ... |
Current - No bullet prefix |
| Multiple errors | {} requires 2 fields |
invalid configuration:- at '': missing property 'replicaCount'(stops at first error) |
multiple errors found: - required field "replicaCount" is missing - required field "image" is missing |
Current - Shows ALL errors! |
Give a look and let me wdyt?
5720563 to
c16f9e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c16f9e0 to
e97183c
Compare
Bundle configuration is now validated using JSONSchema. Configuration errors (typos, missing required fields, wrong types) are caught immediately with clear error messages instead of failing during installation. Assisted-by: Cursor
e97183c to
daa37bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Not a ValidationError, return as-is wrapped | ||
| return fmt.Errorf("invalid configuration: %w", err) |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error wrapping at this line could result in duplicate "invalid configuration:" prefixes in error messages. When this error is returned to UnmarshalConfig (line 134), it will be wrapped again with "invalid configuration: %w", resulting in "invalid configuration: invalid configuration: ..." for non-ValidationError cases.
Consider returning the error without the "invalid configuration:" prefix here, since the caller already adds it:
return err| // Not a ValidationError, return as-is wrapped | |
| return fmt.Errorf("invalid configuration: %w", err) | |
| // Not a ValidationError, return as-is | |
| return err |
When we install an operator using ClusterExtension, we can now configure things like which namespace it should watch. Previously, if you made a typo or configured it incorrectly, the error would only show up later during deployment. Now, configuration is validated immediately using JSON Schema, and we get clear error messages right away.
Why We Did This
Issue: https://issues.redhat.com/browse/OPRUN-4112
Users were getting confusing errors when they misconfigured their operators. By validating configuration upfront, we can give much better error messages that tell you exactly what's wrong and how to fix it.
How It Works
We introduced a
ConfigSchemaProviderinterface that lets different bundle types describe their own configuration rules and all packages format types (registry/v1 or helm) use the same validation process - only the source of the rules changes.Do I Need to Configure
watchNamespace?It depends on what install modes your operator supports:
Examples That Work
No configuration (AllNamespaces mode)
Watch specific namespace (SingleNamespace mode)
Watch install namespace (OwnNamespace mode)
Error Messages
All errors start with
invalid ClusterExtension configuration: invalid configuration:followed by the specific problem.Typo in Field Name
Error:
Missing Required Field
Error:
Wrong Type
Error:
OwnNamespace Mode - Wrong Namespace
Error:
SingleNamespace Mode - Can't Use Install Namespace
Error:
Invalid JSON/YAML
Error:
Technical Details
Design Diagram