Improvement/cldsrv 553 bump deps #5711

benzekrimaha · 2024-12-06T14:42:38Z

bert-e · 2024-12-06T14:42:42Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2024-12-06T14:43:51Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-553 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.8.39

Please check the Fix Version/s of CLDSRV-553, or the target
branch of this pull request.

bert-e · 2024-12-12T14:04:51Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-553 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.9.0

Please check the Fix Version/s of CLDSRV-553, or the target
branch of this pull request.

Dockerfile

francoisferrand · 2024-12-12T16:17:58Z

Dockerfile

+RUN npm install -g node-gyp
 COPY package.json yarn.lock /usr/src/app/
 RUN npm install [email protected] -g


best to install all "global" packages in a single step,
before package.json copy (to avoid invalidatoin),
and specifying the node-gyp version (just like we do for typescript)

Suggested change

RUN npm install -g node-gyp

COPY package.json yarn.lock /usr/src/app/

RUN npm install [email protected] -g

RUN npm install -g \

[email protected] \

[email protected]

COPY package.json yarn.lock /usr/src/app/

lib/api/apiUtils/authorization/prepareRequestContexts.js

bert-e · 2024-12-12T16:29:23Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

lib/Config.js

lib/utapi/utilities.js

francoisferrand · 2024-12-12T16:42:47Z

.github/workflows/tests.yaml

@@ -342,39 +342,6 @@ jobs:
        source: /tmp/artifacts
      if: always()



should be skipped instead of removed:

Suggested change

utapi-v2-tests:

if: false

francoisferrand · 2024-12-18T13:47:16Z

please rebase, there are conflicts.
(sorry, add some pending commensts, which may not be relevant anymore : please with this in mind...)

francoisferrand · 2024-12-18T13:48:51Z

bin/list_metrics.js

@@ -1,4 +0,0 @@
-#!/usr/bin/env node


should keep this file, and just add early return (if needed) in listMetrics()

francoisferrand · 2024-12-18T13:48:58Z

bin/list_bucket_metrics.js

-#!/usr/bin/env node
-'use strict'; // eslint-disable-line strict
-
-require('../lib/utapi/utilities.js').listMetrics('buckets');


should keep this file, and just add early return (if needed) in listMetrics()

Issue: CLDSRV-553

This reverts commit a20bc5d.

lib/Config.js

francoisferrand · 2024-12-26T17:35:24Z

tests/functional/aws-node-sdk/lib/utility/createEncryptedBucket.js

@@ -1,5 +1,5 @@
 const childProcess = require('child_process');
-const Promise = require('bluebird');
+const myPromise = require('bluebird');


rename bluebirdPromise (or bbPromise).... or can we switch to using "standard" node.js Promise instead ?

francoisferrand · 2024-12-26T17:35:40Z

tests/functional/aws-node-sdk/test/bucket/get.js

@@ -1,6 +1,6 @@
 const assert = require('assert');
 const tv4 = require('tv4');
-const Promise = require('bluebird');
+const myPromise = require('bluebird');


francoisferrand · 2024-12-26T17:39:58Z

tests/functional/aws-node-sdk/test/legacy/authV2QueryTests.js

@@ -1,5 +1,5 @@
 const assert = require('assert');
-const process = require('process');
+const myProcess = require('process');


can't we switch to using the new standard process module?

const process = require('node:process');

francoisferrand · 2024-12-26T17:40:36Z

tests/functional/aws-node-sdk/test/legacy/authV4QueryTests.js

@@ -1,5 +1,5 @@
 const assert = require('assert');
-const process = require('process');
+const myProcess = require('process');


francoisferrand · 2024-12-26T17:41:20Z

tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/azureObjectCopy.js

@@ -31,7 +31,7 @@ const bigMD5 = '5f363e0e58a95f06cbe9bbc662c5dfb6';
 const emptyMD5 = 'd41d8cd98f00b204e9800998ecf8427e';
 const locMetaHeader = constants.objectLocationConstraintHeader.substring(11);

-const Promise = require('bluebird');
+const myPromise = require('bluebird');


use default Promise & promisify?

francoisferrand · 2024-12-26T17:41:25Z

tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/objectCopy.js

@@ -19,7 +19,7 @@ const body = Buffer.from('I am a body', 'utf8');
 const correctMD5 = 'be747eb4b75517bf6b3cf7c5fbb62f3a';
 const emptyMD5 = 'd41d8cd98f00b204e9800998ecf8427e';
 const locMetaHeader = constants.objectLocationConstraintHeader.substring(11);
-const Promise = require('bluebird');
+const myPromise = require('bluebird');


use default Promise & promisify?

francoisferrand · 2024-12-26T17:41:36Z

tests/functional/aws-node-sdk/test/legacy/tests.js

@@ -1,5 +1,5 @@
 const assert = require('assert');
-const crypto = require('crypto');
+const myCrypto = require('crypto');


use default node:crypto ?

francoisferrand · 2024-12-26T17:41:48Z

tests/functional/aws-node-sdk/test/multipleBackend/put/put.js

@@ -11,7 +11,7 @@ const { versioningEnabled } = require('../../../lib/utility/versioning-util');
 const { describeSkipIfNotMultiple, getAwsRetry, awsLocation,
    awsLocationEncryption, memLocation, fileLocation, genUniqID, isCEPH }
    = require('../utils');
-const Promise = require('bluebird');
+const myPromise = require('bluebird');


francoisferrand · 2024-12-26T17:41:54Z

tests/functional/aws-node-sdk/test/multipleBackend/utils.js

@@ -1,5 +1,5 @@
 const assert = require('assert');
-const crypto = require('crypto');
+const myCrypto = require('crypto');


francoisferrand · 2024-12-26T17:42:06Z

tests/functional/aws-node-sdk/test/object/copyPart.js

+const myCrypto = require('crypto');

-const Promise = require('bluebird');
+const myPromise = require('bluebird');



francoisferrand · 2024-12-26T17:46:23Z

tests/functional/aws-node-sdk/test/quota/tooling.js

-                reject(result);
-            } else {
-                resolve(result);
+        (async () => {


why do we need this?

since the code is already async/wait, we can simply remove the outer Promise, and keep the function async:

const sendRequest = async (method, host, path, body = '', config = null) => const service = 's3'; const endpoint = new AWS.Endpoint(host); [...] const response = await nodeFetch(url, options); const text = await response.text(); const result = await xml2js.parseStringPromise(text); if (result && result.Error) { throw result; } return result; }

francoisferrand · 2024-12-26T17:49:41Z

tests/functional/metadata/MixedVersionFormat.js

+        after(() => {
+            mongoClient.close(true);
+        });



mongodClient.close() is async (ie returns a Promise), so need to make the function passed to after async as well: too ensure it waits until mongo client is closed.

Suggested change

after(() => {

mongoClient.close(true);

});

after(() => mongoClient.close(true));

or

Suggested change

after(() => {

mongoClient.close(true);

});

after(async () => {

await mongoClient.close(true);

});

francoisferrand · 2024-12-26T17:50:57Z

tests/functional/metadata/MongoClientInterface.js

@@ -44,7 +44,7 @@ const updatedObjVal = { updated: true };
 const runIfMongo =
    process.env.S3METADATA === 'mongodb' ? describe : describe.skip;

-function unescape(obj) {
+function unescapeFunc(obj) {


Suggested change

function unescapeFunc(obj) {

function unescapeJSON(obj) {

francoisferrand · 2024-12-26T17:51:33Z

tests/functional/raw-node/utils/HttpRequestAuthV4.js

@@ -1,11 +1,11 @@
-const crypto = require('crypto');
+const myCrypto = require('crypto');


use node:crypto?

francoisferrand · 2024-12-26T17:51:51Z

tests/functional/s3cmd/tests.js


 const proc = require('child_process');
-const process = require('process');
+const myProcess = require('process');


use node:process ?

francoisferrand · 2024-12-26T17:52:02Z

tests/functional/s3curl/tests.js

+const myProcess = require('process');
 const parseString = require('xml2js').parseString;


francoisferrand · 2024-12-26T17:52:23Z

tests/multipleBackend/routes/routeBackbeat.js

@@ -1,7 +1,7 @@
 const assert = require('assert');
 const AWS = require('aws-sdk');
 const async = require('async');
-const crypto = require('crypto');
+const myCrypto = require('crypto');


use node:crypto?

francoisferrand · 2024-12-26T17:55:23Z

tests/multipleBackend/routes/routeVeeam.js

@@ -1,5 +1,5 @@
 const assert = require('assert');
-const crypto = require('crypto');
+const myCrypto = require('crypto');


francoisferrand · 2024-12-26T17:56:05Z

tests/quota/awsNodeSdk.js

+        before(async () => {
            const config = getConfig('default', { signatureVersion: 'v4', maxRetries: 0 });
            s3Client = new S3(config);
            scuba.start();
-            return metadata.setup(err => wait(2000, () => done(err)));
+            await new Promise((resolve, reject) => {
+                metadata.setup(err => {
+                    if (err) {
+                        reject(err);
+                    }
+                    wait(2000, resolve);
+                });
+            });
        });


Suggested change

before(async () => {

const config = getConfig('default', { signatureVersion: 'v4', maxRetries: 0 });

s3Client = new S3(config);

scuba.start();

return metadata.setup(err => wait(2000, () => done(err)));

await new Promise((resolve, reject) => {

metadata.setup(err => {

if (err) {

reject(err);

}

wait(2000, resolve);

});

});

});

before(() => {

const config = getConfig('default', { signatureVersion: 'v4', maxRetries: 0 });

s3Client = new S3(config);

scuba.start();

return new Promise((resolve, reject) => {

metadata.setup(err => {

if (err) {

reject(err);

}

wait(2000, resolve);

});

});

});

francoisferrand · 2024-12-26T18:04:19Z

tests/unit/api/objectReplicationMD.js

-    isNFS: null,
+    isNFS: undefined,
+};
+const emptyReplicationMDUndef = {


I don't think we put replicationInfo with isNFS: undefined : same (empty) fields are indeed not returned anymore (change in arsenal), but we did not change the way we write the metadata, so the "input" parameter of the tests should not change

Suggested change

const emptyReplicationMDUndef = {

isNFS: null,

};

const expectedEmptyReplicationMD = {

The test is failing otherwise waiting for undefined instead of nul

we did not change the way we write the metadata, so the "input" parameter of the tests should not change

if the test fail, it is probably because we at some point compare to emptyReplicationMD instead of emptyReplicationMDUndef (which would be better renamed expectedEmptyReplicationMD I think)

francoisferrand · 2024-12-26T18:09:19Z

tests/unit/Config.js

@@ -470,7 +470,7 @@ describe('Config', () => {
            process.env.S3_CONFIG_FILE = oldConfig;
        });

-        it('should set up utapi local cache', () => {
+        it.skip('should set up utapi local cache', () => {


this should make linter fail : as it does not, it seems something is missing from the config :-(
so we'll need to think about adding it later...

francoisferrand

LGTM overall, but a few comments have not been adressed yet (esp. in gha)

francoisferrand · 2025-01-08T13:54:17Z

Dockerfile

+RUN npm install -g node-gyp
 COPY package.json yarn.lock /usr/src/app/
 RUN npm install [email protected] -g


lib/Config.js

francoisferrand · 2025-01-08T13:56:44Z

lib/Config.js

 } = require('../constants');
-const { utapiVersion } = require('utapi');
+// const { utapiVersion } = require('utapi');


"mock" the variable definition, so you don't need to add eslint-disable-line no-undef

Suggested change

// const { utapiVersion } = require('utapi');

const utapiVersion = 0; // require('utapi');

tests/functional/aws-node-sdk/test/multipleBackend/objectCopy/azureObjectCopy.js

francoisferrand · 2025-01-08T14:05:09Z

tests/functional/aws-node-sdk/test/quota/tooling.js

-const sendRequest = async (method, host, path, body = '', config = null) =>
-    new Promise(async (resolve, reject) => {
+const sendRequest = async (method, host, path, body = '', config = null) => {
+
        const service = 's3';


extra empty line

bad indent: should be just 4 spaces

francoisferrand · 2025-01-08T14:10:43Z

tests/quota/awsNodeSdk.js

+            return new Promise((resolve, reject) => {
+                metadata.setup(err => {
+                    if (err) {
+                        reject(err);


missing return

francoisferrand · 2025-01-08T14:17:15Z

tests/quota/awsNodeSdk.js

+                metadata.setup(err => {
+                    if (err) {
+                        reject(err);
+                    }
+                    wait(2000, resolve);
+                });
+            });


do we really need to change the callback-based hook here?
as we are still using only callback-based functions internally, the updated code is more complex than the old one: if no constraint, may be better to keep it until those functions have been updated

francoisferrand · 2025-01-08T14:24:07Z

tests/unit/api/objectReplicationMD.js

-    isNFS: null,
+    isNFS: undefined,
+};
+const emptyReplicationMDUndef = {


we did not change the way we write the metadata, so the "input" parameter of the tests should not change

if the test fail, it is probably because we at some point compare to emptyReplicationMD instead of emptyReplicationMDUndef (which would be better renamed expectedEmptyReplicationMD I think)

francoisferrand · 2025-01-16T09:04:39Z

tests/multipleBackend/routes/routeBackbeat.js

            bucketUtil.emptyIfExists(BUCKET_FOR_NULL_VERSION)
                .then(() => s3.createBucket({ Bucket: BUCKET_FOR_NULL_VERSION }).promise())
-                .then(() => done(), err => done(err))
        );


i don't understand how this code "waits" for the bucket to be created :(
(it would be the case if promise was returned, but i don't see how it is here...)

benzekrimaha force-pushed the improvement/CLDSRV-553-bump-deps branch from dcc7c34 to 0eadb81 Compare December 6, 2024 15:59

francoisferrand changed the base branch from development/8.8 to development/8.9 December 12, 2024 14:04

francoisferrand reviewed Dec 12, 2024

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

francoisferrand reviewed Dec 12, 2024

View reviewed changes

lib/api/apiUtils/authorization/prepareRequestContexts.js Show resolved Hide resolved

benzekrimaha force-pushed the improvement/CLDSRV-553-bump-deps branch 2 times, most recently from 15b93ac to 57325de Compare December 12, 2024 16:26

benzekrimaha marked this pull request as ready for review December 12, 2024 16:26

benzekrimaha force-pushed the improvement/CLDSRV-553-bump-deps branch from 57325de to 2639bf6 Compare December 12, 2024 16:29

scality deleted a comment from bert-e Dec 12, 2024

benzekrimaha force-pushed the improvement/CLDSRV-553-bump-deps branch from 8bb9b0a to b5e6e01 Compare December 12, 2024 16:49

francoisferrand reviewed Dec 18, 2024

View reviewed changes

benzekrimaha added 11 commits December 20, 2024 08:57

Bump dependencies

7bfe32a

Issue: CLDSRV-553

Adapt docker file

48b4a6a

Issue: CLDSRV-553

Adapt config post bump eslint to v9

8e6a32f

Issue: CLDSRV-553

Update code to match scality guidelines

a942098

Issue: CLDSRV-553

Md lint fixup

5b2d51c

Issue: CLDSRV-553

Adapt package.json mocha tests commands

1f72d50

Issue: CLDSRV-553

Tests fixups post bump failures

6522b62

Issue: CLDSRV-553

Bump node version in gha

b60f93f

Issue: CLDSRV-553

Skipping tests not working

f1fdb1c

Issue: CLDSRV-553

Revert "[to be reverted]utapi removal"

f03db55

This reverts commit a20bc5d.

test

f1a4f29

francoisferrand reviewed Dec 26, 2024

View reviewed changes

lib/Config.js Outdated Show resolved Hide resolved

francoisferrand reviewed Dec 26, 2024

View reviewed changes

lib/Config.js Outdated Show resolved Hide resolved

francoisferrand reviewed Dec 26, 2024

View reviewed changes

lib/Config.js Outdated Show resolved Hide resolved

francoisferrand reviewed Dec 26, 2024

View reviewed changes

benzekrimaha added 3 commits January 3, 2025 16:46

deps update

d608869

post reviews fixups

77788e3

post reviews fixups

399e3e9

francoisferrand reviewed Jan 8, 2025

View reviewed changes

fixups post reviews

d06ec25

francoisferrand reviewed Jan 16, 2025

View reviewed changes

		@@ -342,39 +342,6 @@ jobs:
		source: /tmp/artifacts
		if: always()

		@@ -1,11 +1,11 @@
		const crypto = require('crypto');
		const myCrypto = require('crypto');

		const myProcess = require('process');
		const parseString = require('xml2js').parseString;

	// const { utapiVersion } = require('utapi');
	const utapiVersion = 0; // require('utapi');

Improvement/cldsrv 553 bump deps #5711

Are you sure you want to change the base?

Improvement/cldsrv 553 bump deps #5711

Conversation

benzekrimaha commented Dec 6, 2024 • edited Loading

bert-e commented Dec 6, 2024

Hello benzekrimaha,

bert-e commented Dec 6, 2024

Incorrect fix version

bert-e commented Dec 12, 2024

Incorrect fix version

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bert-e commented Dec 12, 2024

Waiting for approval

Choose a reason for hiding this comment

francoisferrand commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoisferrand Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoisferrand Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

francoisferrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benzekrimaha commented Dec 6, 2024 •

edited

Loading

francoisferrand Dec 26, 2024 •

edited

Loading

francoisferrand Dec 26, 2024 •

edited

Loading