-
Notifications
You must be signed in to change notification settings - Fork 20
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
Improvement/bb 585 bump deps #2611
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
@@ Coverage Diff @@
## development/9.0 #2611 +/- ##
===================================================
- Coverage 71.15% 71.14% -0.01%
===================================================
Files 201 201
Lines 13336 13333 -3
===================================================
- Hits 9489 9486 -3
Misses 3837 3837
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. |
9b15fe7
to
ab81bc9
Compare
955428e
to
0a60952
Compare
/approve |
ae210a1
to
1cc4d4d
Compare
f55a254
to
b148588
Compare
6bb4676
to
70729d1
Compare
b148588
to
f599fc9
Compare
70729d1
to
60e09a3
Compare
f599fc9
to
cb0a2e3
Compare
@@ -242,19 +242,19 @@ describe('Lifecycle Conductor', () => { | |||
}); | |||
|
|||
describe('_indexesGetOrCreate', () => { | |||
it('should return v1 for bucketd bucket sources', () => { | |||
it('should return v2 for bucketd bucket sources', () => { |
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.
should not have changed, we should have the same behavior as before the node22 bump?
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 initial test case was false, there was a bug in the code that was fixed during the unification (with the previous code, S3C never used the v2 listing) the tests were missed as they didn't fail after the change as they never called done() after _indexesGetOrCreate
finished executing.
Bumping the "assert" package made the tests fail, not sure what the exact reason is, might be a performance increase or something else but now the assert function returns before the test ends.
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.
OK, but can we double-check with hopocalipse that we have an issue in the tests (i.e. bucketd must indeed use V2), or is there an issue in the lib (i.e. we should use V1 for bucketd, but we use V2) : maybe s3c should not use v2 listing, and it was done on purpose... aka activating v2 listing in s3c should not be a silent improvement thanks to the unification, but a conscious choice of which everybody should be aware.
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.
In the 7.x branch, listing v1 is only used when forcing it using the forceLegacyListing
flag in the config. We kept the same behaviour in 9.x
d6c0c49
to
1347c61
Compare
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
28050cf
to
69e9fd9
Compare
69e9fd9
to
dcfde66
Compare
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue BB-585. Goodbye benzekrimaha. The following options are set: approve |
Issue: BB-585