🎨 Async/Await migration on bucketGet and objectGetLegalHold#6045
🎨 Async/Await migration on bucketGet and objectGetLegalHold#6045DarkIsDude wants to merge 5 commits intofeature/CLDSRV-823/formatting-onlyfrom
Conversation
Hello darkisdude,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
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## feature/CLDSRV-823/formatting-only #6045 +/- ##
===================================================================
Coverage 84.45% 84.45%
===================================================================
Files 206 206
Lines 13164 13186 +22
===================================================================
+ Hits 11117 11136 +19
- Misses 2047 2050 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
16402cf to
4e7d644
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
bafa722 to
08d8dc0
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
08d8dc0 to
2196def
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
2196def to
250de6f
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
474bad8 to
7a14efc
Compare
lib/api/objectGetLegalHold.js
Outdated
|
|
||
| const { convertToXml } = s3middleware.objectLegalHold; | ||
|
|
||
| const standardMetadataValidateBucketAndObjPromised = (metadataValParams, actionImplicitDenies, log) => |
There was a problem hiding this comment.
Instead of doing one custom promisify for each file that is migrated, we can do the same thing in another way:
https://nodejs.org/docs/latest-v22.x/api/util.html#custom-promisified-functions
Doing it at function level will facilitate its migration and reduce impacts.
There was a problem hiding this comment.
we should also add this trick to the confluence page (in the tips & tricks part)
There was a problem hiding this comment.
Thanks for this trick. Discover it, that's perfect for what we are doing. You can review this commit 872cfa3 and the confluence page where I added your idea 👏 https://scality.atlassian.net/wiki/spaces/OS/pages/3523346468/2025-10-30+-+Async+Await+migration#%E2%9A%A0%EF%B8%8F-Limitation%3A-Multiple-callback-arguments
| throw errors.NoSuchObjectLockConfiguration; | ||
| } | ||
|
|
||
| const additionalResHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket); |
There was a problem hiding this comment.
To keep the initial behavior, this function should be call earlier to do the err.additionalResHeader trick.
There was a problem hiding this comment.
@maeldonn can you double check ? The err was not added on all errors but only one some. I rechecked and I think I followed the same path as before. Did I miss something ?
There was a problem hiding this comment.
Sorry I think I confused with the bucketGet.js file... Just ignore my comment 😅
lib/api/objectGetLegalHold.js
Outdated
|
|
||
| const { convertToXml } = s3middleware.objectLegalHold; | ||
|
|
||
| const standardMetadataValidateBucketAndObjPromised = (metadataValParams, actionImplicitDenies, log) => |
There was a problem hiding this comment.
we should also add this trick to the confluence page (in the tips & tricks part)
7a14efc to
c98d577
Compare
c98d577 to
4f6b182
Compare
d6342e9 to
7559941
Compare
7559941 to
99b9108
Compare
872cfa3 to
7bed447
Compare
Issue: CLDSRV-823
Description
Motivation and context
Opening a first PR to migrate from callback to async/await syntax. A TAD is opened.
https://scality.atlassian.net/wiki/spaces/OS/pages/3523346468/2025-10-30+-+Async+Await+migration
This is a first PR to start the migration. As mention in the TAD, the idea is to migrate to async/await when working on a specific part of our component. Maybe a common goal of one PR per squad per sprint is a good idea.
All idea or remarks are welcome 🙏