CLDSRV-965: checksums feature flag - #6248
Conversation
Hello leif-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.4 #6248 +/- ##
===================================================
+ Coverage 86.32% 86.36% +0.03%
===================================================
Files 212 212
Lines 14528 14562 +34
===================================================
+ Hits 12541 12576 +35
+ Misses 1987 1986 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
064d136 to
d2f1698
Compare
d2f1698 to
8218847
Compare
dvasilas
left a comment
dvasilas
left a comment
There was a problem hiding this comment.
As discussed, there are cases when completeMultipartUpload could permanently fail because parts have been enabled with checksums disabled (during upgrades, if checksums are disabled and then re-enabled).
Let's create a documentation ticket to document these cases and how they should be resolved.
I think I will add an error log |
Makes sense 👍 . That's a better choice than requiring a manual intervention for each problematic MPU. |
| JOB_NAME: ${{ github.job }} | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
nit: checkout v4 is getting old 🧐 v7 available
| case ChecksumError.MPUTypeNotConfigured: | ||
| return errMPUTypeNotConfigured; | ||
| case ChecksumError.MPUTypeModeMismatch: | ||
| return errorInstances.InvalidRequest.customizeDescription( |
There was a problem hiding this comment.
nit: a bit weird that you bothered defining errMPUTypeNotConfigured at the top of the file, but not this error 🤔 If it's because you need the details.type, you can still define it at the top with a function that take a param and return the error
There was a problem hiding this comment.
nvm its already whats done for other custom error 🤷
There was a problem hiding this comment.
All errors have a cloudserver definition for unit tests, in this case ChecksumError.MPUTypeNotConfigured only maps to errMPUTypeNotConfigured, but sometimes ChecksumError share the same arsenal error.
The goal is to separate domain errors from XML arsenal / AWS S3 error messages
| if (config && config.integrityChecks) { | ||
| assert( | ||
| typeof config.integrityChecks === 'object' && !Array.isArray(config.integrityChecks), | ||
| 'bad config: integrityChecks must be an object', | ||
| ); | ||
|
|
There was a problem hiding this comment.
nit: isn't this kind of stuff preferably handled by the Joi library directly 🤔
There was a problem hiding this comment.
I think its doable with joi, with like joi.default({enabled: true}) or something
But honestly whatever, half the file is joi and half the file is manual check
There was a problem hiding this comment.
Yea, I followed the existing pattern
| return res; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This comment looks too verbose, it concise
There was a problem hiding this comment.
It is verbose but it documents what the feature flag does and doesn't do. There is no useless info in it
5 participants
Content-MD5etx-amz-checksum-sha256not impacted by the flag