feat: implement strict 500 version limit for configurations #1153
Conversation
|
This PR will trigger a minor release when merged. |
| this.log.info(`Enforcing configuration version limit: deleting ${deleteCount} old versions (current: ${allConfigs.length}, target: ${MAX_VERSIONS})`); | ||
|
|
||
| // Delete in batches (DynamoDB batch write limit is 25) | ||
| for (let i = 0; i < versionsToDelete.length; i += BATCH_SIZE) { |
There was a problem hiding this comment.
Do we need batching.
I mean before pushing it to production , we must clean up manually (DB script) to reach to base line.
I don't want to do first clean up by service it self.
There was a problem hiding this comment.
I have create a new script to delete the versions before deploying to code. with this we will only keep 500 version and then whenever a new version is saved then it will check is the number is greater that 500 then it will remove the oldest version
| const ids = batch.map((config) => config.getId()); | ||
|
|
||
| // eslint-disable-next-line no-await-in-loop | ||
| await this.removeByIds(ids); |
There was a problem hiding this comment.
can't we leave it async ?
There was a problem hiding this comment.
I've simplified the implementation to remove batching since we'll typically only delete 1-2 versions in normal operation. The new code eliminates the loop entirely, so the eslint disable is no longer needed. The cleanup now does a single removeByIds call with all IDs at once."
packages/spacecat-shared-data-access/src/models/configuration/configuration.collection.js
Outdated
Show resolved
Hide resolved
|
@tathagat2241 |
|
@ravverma, Yes, we will not be doing it for 4 weeks reason being there could be a instance that during 4 weeks only a few versions are created so it will default to keep 100 version, which I feel is a pretty low number. During the event like Adobe summit we do onboard a lot of new sites and during times like these just keeping 100 versions would not be safe. So, decided to keep it simple and always store 500 version at every instance |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }); | ||
|
|
||
| // Verify Configuration entity is available | ||
| if (!dataAccess || !dataAccess.Configuration) { |
Check notice
Code scanning / CodeQL
Unneeded defensive code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, remove the unneeded defensive check for !dataAccess in the conditional on line 73. Only the !dataAccess.Configuration check should remain, as that's the relevant error case. Specifically, change the code from:
if (!dataAccess || !dataAccess.Configuration) {to:
if (!dataAccess.Configuration) {No changes are needed elsewhere in the file, and no new imports or definitions are required.
| @@ -70,7 +70,7 @@ | ||
| }); | ||
|
|
||
| // Verify Configuration entity is available | ||
| if (!dataAccess || !dataAccess.Configuration) { | ||
| if (!dataAccess.Configuration) { | ||
| console.error('ERROR: Configuration entity is not available'); | ||
| console.error(' Troubleshooting:'); | ||
| console.error(' 1. Ensure DYNAMO_TABLE_NAME is set correctly'); |
Changes
#enforceVersionLimit()method toConfigurationCollectionthat automatically cleans up old versions on everysave()operationsetImmediate()to avoid blocking the main save operationTechnical Details
create()(save)Promise.all()Please ensure your pull request adheres to the following guidelines:
Related Issues
https://jira.corp.adobe.com/browse/SITES-37650
Thanks for contributing!