-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add configuration management APIs #1063
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
|
This PR will trigger a minor release when merged. |
ramboz
left a comment
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.
Looks overall good to me
ravverma
left a comment
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.
@tathagat2241 minor comments, please address
| throw new Error(`Handler "${type}" not found in configuration`); | ||
| } | ||
|
|
||
| if (properties.productCodes !== undefined) { |
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.
use hasText
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 hasText utility is specifically for strings (isString(str)). This field is a array so hasText would incorrectly return false for valid values
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.
isNonEmptyArray can use this
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.
we will have to use both because Without !== undefined: All fields become required and we don't want that. eg: it will fail when only updating jobs.
And without isNonEmpty*(): Empty values pass through (e.g., {handlers: {}} would be accepted)
| } | ||
| } | ||
|
|
||
| if (properties.dependencies !== undefined) { |
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.
use hasText
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 hasText utility is specifically for strings (isString(str)). This field is a array so hasText would incorrectly return false for valid values.
| } | ||
| } | ||
|
|
||
| if (properties.movingAvgThreshold !== undefined && properties.movingAvgThreshold < 1) { |
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.
use hasText
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 hasText utility is specifically for strings (isString(str)). This field is a NUMBER so hasText would incorrectly return false for valid values.
| throw new Error('Configuration data cannot be empty'); | ||
| } | ||
|
|
||
| if (data.handlers !== undefined) { |
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.
use hasText
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 hasText utility is specifically for strings (isString(str)). This field is a OBJECT so hasText would incorrectly return false for valid values.
| this.setHandlers(mergedHandlers); | ||
| } | ||
|
|
||
| if (data.jobs !== undefined) { |
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.
hasText
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 hasText utility is specifically for strings (isString(str)). This field is a array so hasText would incorrectly return false for valid values.
| this.setJobs(mergedJobs); | ||
| } | ||
|
|
||
| if (data.queues !== undefined) { |
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.
use hasText
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 hasText utility is specifically for strings (isString(str)). This field is a OBJECT so hasText would incorrectly return false for valid values.
Summary
This PR adds new configuration management methods to support API-driven configuration updates
Changes Made
Configuration Model (
configuration.model.js)updateQueues(queues)- Update queue configurationsupdateJob(type, properties)- Update job properties (interval, group)updateHandlerProperties(type, properties)- Update handler settings (enabledByDefault, productCodes, dependencies, thresholds)updateConfiguration(data)- Flexible method to update multiple configuration sections at onceproductCodesa required and non-empty field for handlersrestore(version)- restores the last good versionregister(audit)andunregister(audit)- remove AUDIT_TYPES static list validation.Configuration Schema (
configuration.schema.js)productCodeswith minimum 1 itemConfiguration Index (
index.js)checkConfigurationfunction for response validationTesting
Please ensure your pull request adheres to the following guidelines:
Related Issues
https://jira.corp.adobe.com/browse/LLMO-908
Thanks for contributing!