S3UTILS-217: improve listObjectsByReplicationStatus.js #367
Conversation
- Show details of the errors when they happen - Display the bucket's name - Display the IsLatest field
Hello scality-gdoumergue,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1 #367 +/- ##
=================================================
+ Coverage 43.76% 43.78% +0.02%
=================================================
Files 84 84
Lines 5962 5966 +4
Branches 1256 1256
=================================================
+ Hits 2609 2612 +3
- Misses 3307 3308 +1
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code itself LGTM, nothing to say here.
But, recently I spent some time to make sure we can test this script in the CI, previously there was none. So if you have the time in hand, and can have a look at this file and if you are able to add some tests that would be great. If not, no worries, I'll take note and create a ticket on my end to do so.
Some notes:
Those tests do require a tool we created to simulate a small s3c env called workbench, to install it:
curl -L -o /tmp/workbench.tar.gz https://github.com/scality/workbench/releases/download/v0.8.0/workbench_Linux_x86_64.tar.gz
tar -xzf /tmp/workbench.tar.gz -C /tmp
sudo install /tmp/workbench /usr/local/bin/workbenchRun it inside s3utils repo, like so:
workbench up --env-dir ./workbench/env -dYou'll then be able to run those particular test like so:
yarn jest --verbose --logHeapUsage --projects jest.config.js --coverage --testPathPattern='tests/functional/listObjectsByReplicationStatus.js'Again, only if you have the time in hand, just let me know.
|
Thanks @tcarmet , I will use workbench to improve my knowledge! |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
scality-gdoumergue
force-pushed
the
improvement/S3UTILS-217/listObjectsByReplicationStatus.js-v2
branch
from
c493b16 to
116f756
Compare
scality-gdoumergue
force-pushed
the
improvement/S3UTILS-217/listObjectsByReplicationStatus.js-v2
branch
from
116f756 to
9e35963
Compare
| return originalError(message, data); | ||
| }; | ||
|
|
||
| const endpoint = `http://${s3Host}:${s3Port}`; |
There was a problem hiding this comment.
Nit: I'm assuming I made this mistake in the first existing test but we might as well add it as a const in the beforeAll (or Each) to avoid redeclaring it.
| // Clean up | ||
| // Delete all objects in the non-replicated bucket | ||
| const versionResp = await accountSource.s3Client.send(new ListObjectVersionsCommand({ Bucket: nonRepBucketName })); | ||
| if (versionResp.Versions && versionResp.Versions.length > 0) { | ||
| const objectsToDelete = versionResp.Versions.map(v => ({ | ||
| Key: v.Key, | ||
| VersionId: v.VersionId, | ||
| })); | ||
| await accountSource.s3Client.send(new DeleteObjectsCommand({ | ||
| Bucket: nonRepBucketName, | ||
| Delete: { | ||
| Objects: objectsToDelete | ||
| } | ||
| })); | ||
| } | ||
| await accountSource.s3Client.send(new DeleteBucketCommand({ Bucket: nonRepBucketName })); |
There was a problem hiding this comment.
Should belong in a afterEach, can be part of of the deleteAccount function (I think there's already something that deletes objects that can be improved to support those deletes).
| expect(foundKeys).toContain(testObj.Key); | ||
| // Expect the bucketName to be included in the log entry | ||
| const logEntry = foundObjectLogs.find(e => e.data.Key === testObj.Key); | ||
| expect(logEntry.data.bucketName).toBe(accountSource.bucketName); |
This script has been tested in a lab: