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.

francoisferrand · 2024-12-12T16:14:33Z

Dockerfile

@@ -1,4 +1,4 @@
-ARG NODE_VERSION=16.20.2-bookworm-slim
+ARG NODE_VERSION=22-bookworm-slim


should stick to a specific node version, to ensure reproductible & repeatable builds

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/

francoisferrand · 2024-12-12T16:19:59Z

lib/api/apiUtils/authorization/prepareRequestContexts.js

@@ -154,13 +154,6 @@ function prepareRequestContexts(apiMethod, request, sourceBucket,
                generateRequestContext('objectPutTaggingVersion');
            requestContexts.push(putObjectVersionRequestContext);
        }


why are we changing permissions?
if it was (already) a bug, it shoudl be fixed on 8.8 as well instead here...

@benzekrimaha did you miss this one?

it's not changing the permission ,this code is actually duplicated see here : https://github.com/scality/cloudserver/pull/5711/files#diff-465d8f1849b67c1a40847391c0a7ea074b6357a8b7cdeb2db2be89ad9dfce50bL79-L80

we are never going to cross this condition

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.

francoisferrand · 2024-12-26T17:18:17Z

.github/workflows/tests.yaml

@@ -149,7 +149,7 @@ jobs:
      if: always()

  build:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-latest


best to specify the version explicitely (24.04)

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

lib/api/bucketPutACL.js

@@ -261,6 +263,9 @@ function bucketPutACL(authInfo, request, log, callback) {
                        return next(null, bucket, revisedAddACLParams);
                    });
            }
+            if (hasError) {


should be earlier in the code (eright after the jsonGrants.forEach loop) : otherwise we may continue while we had an error, and risk calling the callback again (like on line 263)

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

lib/api/objectPutACL.js

@@ -272,6 +274,9 @@ function objectPutACL(authInfo, request, log, cb) {
                            revisedAddACLParams);
                    });
            }
+            if (hasError) {


same, should be called earlier : to avoid having a duplicate call via line 273...

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

lib/utapi/utapiReindex.js

+// const { config } = require('../Config');

-const reindexConfig = config.utapi && config.utapi.reindex;
-if (reindexConfig && reindexConfig.password === undefined) {
-    reindexConfig.password = config.utapi && config.utapi.redis
-    && config.utapi.redis.password;
-}
-const reindex = new UtapiReindex(reindexConfig);
-reindex.start();
+// const reindexConfig = config.utapi && config.utapi.reindex;
+// if (reindexConfig && reindexConfig.password === undefined) {
+//     reindexConfig.password = config.utapi && config.utapi.redis
+//     && config.utapi.redis.password;
+// }


no need to comment these

francoisferrand · 2024-12-26T17:30:22Z

lib/utapi/utapiReplay.js

+// const _config = require('../Config').config;

-const utapiConfig = _config.utapi &&
-    Object.assign({}, _config.utapi, { redis: _config.redis });
-const replay = new UtapiReplay(utapiConfig); // start utapi server
-replay.start();
+// const utapiConfig = _config.utapi &&
+//     Object.assign({}, _config.utapi, { redis: _config.redis });


no need to comment

francoisferrand · 2024-12-26T17:30:59Z

lib/Config.js

+const { azureAccountNameRegex, base64Regex,
+    allowedUtapiEventFilterFields, allowedUtapiEventFilterStates,
+    supportedLifecycleRules
 } = require('../constants');


change not needed?

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

lib/Config.js

@@ -1236,7 +1233,7 @@ class Config extends EventEmitter {
            maxStaleness,
            enableInflights,
        };
-        if (config.utapi) {
+        if (config.utapi && false) { // eslint-disable-line no-constant-condition


no need to change, will work in case we don't have utapi?

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

lib/Config.js

        if (Object.keys(this.locationConstraints).some(
-        loc => this.locationConstraints[loc].sizeLimitGB)) {
+        loc => this.locationConstraints[loc].sizeLimitGB) && false) {


no need to 'fase' this, we should never run into this code anyway

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

lib/Config.js

@@ -3,7 +3,7 @@ const { EventEmitter } = require('events');
 const fs = require('fs');
 const path = require('path');
 const url = require('url');
-const crypto = require('crypto');
+const mycrypto = require('crypto');


why rename?
can't we use node22's new built-in crypto module?

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 = {

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...

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

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

		@@ -1,4 +1,4 @@
		ARG NODE_VERSION=16.20.2-bookworm-slim
		ARG NODE_VERSION=22-bookworm-slim

		@@ -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;

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

francoisferrand Dec 12, 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

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

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

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

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

francoisferrand Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

benzekrimaha commented Dec 6, 2024 •

edited

Loading

francoisferrand Dec 12, 2024 •

edited

Loading

francoisferrand Dec 26, 2024 •

edited

Loading

francoisferrand Dec 26, 2024 •

edited

Loading

francoisferrand Dec 26, 2024 •

edited

Loading

francoisferrand Dec 26, 2024 •

edited

Loading