-
Notifications
You must be signed in to change notification settings - Fork 2
S3UTILS-217: improve listObjectsByReplicationStatus.js #367
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: development/1
Are you sure you want to change the base?
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.73% -0.03%
=================================================
Files 84 84
Lines 5962 5966 +4
Branches 1256 1256
=================================================
Hits 2609 2609
- Misses 3307 3311 +4
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
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.
| Key, | ||
| VersionId, | ||
| IsLatest, | ||
| error: err |
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.
This can help logging error messages when the message field is dynamically computed:
| error: err | |
| error: err.message |
| }, err => { | ||
| if (err) { | ||
| log.error('error processing batch of objects', { | ||
| error: err, |
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.
same here
This script has been tested in a lab: