✨ S3UTILS-230 implement CRR multi-site V2 replication format#396
✨ S3UTILS-230 implement CRR multi-site V2 replication format#396DarkIsDude wants to merge 14 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1 #396 +/- ##
=================================================
+ Coverage 44.90% 45.31% +0.41%
=================================================
Files 88 88
Lines 6456 6516 +60
Branches 1352 1369 +17
=================================================
+ Hits 2899 2953 +54
- Misses 3511 3513 +2
- Partials 46 50 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d5490b8 to
f9fca0c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
maeldonn
left a comment
There was a problem hiding this comment.
Can be simplified a lot by using arsenal logic
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
2649e03 to
c03ea05
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
maeldonn
left a comment
There was a problem hiding this comment.
One final batch of reviews and we should be good to go 💪
| */ | ||
| _isV1Format(repConfig) { | ||
| return repConfig.Rules.every(r => !r.Filter); | ||
| } |
There was a problem hiding this comment.
Used once, could be inlined to simplify code reading.
There was a problem hiding this comment.
It was intended here. As the function is waterflow, with old style and very long, I specifically tried to split it. If it's a hard no for you, I can change 🙏
| _computeTopLevelStatus(backends) { | ||
| if (backends.some(b => b.status === 'FAILED')) { return 'FAILED'; } | ||
| if (backends.some(b => b.status === 'PENDING')) { return 'PROCESSING'; } | ||
| return 'COMPLETED'; | ||
| } |
There was a problem hiding this comment.
Since this is only used once, it could be inlined. While extracting logic to an external function usually improves readability, creating small functions for single-use cases can actually make the code harder to follow
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
d1e9317 to
dcbdaa0
Compare
Summary
Changes
`CRR/ReplicationStatusUpdater.js`
`package.json` — arsenal pinned to `improvement/ARSN-571/crr-multi` for the new static methods
`tests/utils/crr.js` — V2 bucket replication config fixture
`tests/unit/CRR/ReplicationStatusUpdater.js` — 7 new V2 test cases (single match, dual-rule prefix overlap, no-match skip, SITE_NAME filter, all-up-to-date skip, PROCESSING aggregate status, forceUsingConfiguration)
Test plan
Issue: S3UTILS-230
:80. Should we ping CS or update the page directly ?