-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Feature Better configuration management #25652
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?
Conversation
|
TypeScript types have been updated based on the JSON schema changes in the PR |
|
TypeScript types have been updated based on the JSON schema changes in the PR |
…' into feature-configuration-management
Code Review 👍 Approved with suggestions 1 resolved / 3 findingsWell-designed configuration source management feature with ENV/DB/AUTO modes. Both previous findings remain unresolved: envSyncTimestamp parameter still always null, and IllegalArgumentException still used for config protection (HTTP 400 vs 403). 💡 Bug: envSyncTimestamp parameter is always null in shouldUseEnvValue call📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/settings/SettingsCache.java In the boolean shouldUseEnv =
ConfigSourceResolver.shouldUseEnvValue(
configSource,
currentEnvHash,
storedEnvHash,
null, // envSyncTimestamp is always null
dbModifiedTimestamp,
applicationStartTime);While the current implementation of Impact: Low currently since the parameter isn't used, but this could cause issues if the logic is extended to use 💡 Quality: IllegalArgumentException may not be appropriate HTTP status for config protection📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java 📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/system/SystemResource.java When a user tries to update an ENV-managed configuration, the code throws
Suggested fix: Consider using a more specific exception that maps to the appropriate HTTP status code, such as:
This would provide clearer semantics to API consumers about why their request was rejected. ✅ 1 resolved✅ Edge Case: Incomplete SettingsType handling in convertToOpenMetadataConfig
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|



Adding a configSource property to OpenMetadata settings that lets operators control where
configuration is read from:
updates)
Problem solved: Previously, settings stored in the database on first startup would ignore
subsequent ENV/YAML changes, confusing operators who expected config file changes to take effect.
Scope: