refactor env parameters by SylvainSenechal · Pull Request #2394 · scality/Zenko

SylvainSenechal · 2026-04-24T15:19:46Z

Mostly refactoring around tests configuration.

Lots of tests parameters are shared between mocha and cucumber tests => setup a configuration folder that can be used by both test suite to load tests parameters
Trying to offload the work from bash script directly into the tests
Many parameters are still not added in this pr, because :
- the pr would become too big
- some parameters are still used in bash or python scripts => Putting them in the js files would not allow us to remove them from where they currently sit, so we would end up with duplicated declarations which is worse. Longer term we have tickets to migrate python setup scripts into the tests, so this migration can be done at that time
- We have a fully typed configuration clean object holding all the tests variable/configuration

bert-e · 2026-04-24T15:19:50Z

Hello sylvainsenechal,

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 · 2026-04-30T14:11:30Z

Waiting for approval

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

the author
2 peers

bert-e · 2026-04-30T16:25:52Z

Waiting for approval

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

the author
2 peers

bert-e · 2026-05-05T17:23:47Z

Waiting for approval

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

the author
2 peers

Issue: ZENKO-5259

claude · 2026-05-20T18:36:42Z

+    if (Object.keys(config).length > 0) return;
+    const zenkoAccountCredentials = await loadZenkoAccount();
+    const clients = createClients(zenkoAccountCredentials);
+    Object.assign(config, {


initConfig() is all-or-nothing: if any single K8s resource lookup fails (e.g., loadKafkaTopics when the cold-sorbet-config secret doesn't exist), the Object.assign never executes and config stays empty — losing even the resources that did load successfully (like admin/account credentials).

Since mocha tests now also call initConfig() via the root hook, any mocha test suite running in an environment without the full Zenko stack (no sorbet, no Zenko CR) will fail at startup, even though those tests only need account/admin credentials.

Consider wrapping each loader independently so partial failures don't prevent the rest of the config from being populated, or splitting the init into required-vs-optional groups.

yep and im fine with it.
wrapping each loader would means that every time someone wants to use a field in this config, they first have to wonder "is this gonna be undefined because im in this particular test or not", and have some weird failures later down the line because some variable is undefined
I don't want that. Good long term goal is one zenko setup for all tests

basically fail fast and be explicit

claude · 2026-05-20T18:36:44Z

+import { NodeHttpHandler } from '@smithy/node-http-handler';
+import { getSecretByLabel, getSecretByName, getCustomObject } from "./kubernetes";
+
+const ZENKO_NAME = 'end2end';


ZENKO_NAME is hardcoded to 'end2end', but the shell script (setup-e2e-env.sh) accepts it as a positional argument (${1:-end2end}). This removes the ability to run tests against a differently-named Zenko installation. Consider reading from process.env.ZENKO_NAME with a default.

Suggested change

const ZENKO_NAME = 'end2end';

const ZENKO_NAME = process.env.ZENKO_NAME || 'end2end';

kinda good catch but actually what im gonna change here is the positional argument in setup e2e, which is never used, so im removing it

we should keep it.
goal (eventually) is to be able to run against any zenko instance (e.g. artesca), so best to keep it: it does not hurt.

Ok I kept it but current state is not super nice : grep 'end2end' returns 164 results so if we want to use that variable zenko name there are many places that will need to change

SylvainSenechal · 2026-05-20T21:23:08Z

Review the /tests_common folder first, at the end of this pr

SylvainSenechal · 2026-05-20T21:23:38Z


 . "$_SETUP_DIR/common.sh"

-ZENKO_NAME="${ZENKO_NAME:-end2end}"


argument never provided so removing ambiguity

SylvainSenechal · 2026-05-20T21:24:13Z

-    export STORAGE_ACCOUNT_OWNER="${STORAGE_ACCOUNT_OWNER_USER_NAME}"
-    export DATA_CONSUMER="${DATA_CONSUMER_USER_NAME}"
-    export DATA_ACCESSOR="${DATA_ACCESSOR_USER_NAME}"
+    export ACCOUNT="zenko-ctst"


some stuffs like this needs to stay here, they are used by cli-testing, or are coupled with other script/logic and can't be migrated easily without some other preliminary work

SylvainSenechal · 2026-05-20T21:26:12Z

    }
-    Identity.useIdentity(IdentityEnum.ACCOUNT, world.getSaved<string>('accountName') ||
-        world.parameters.AccountName);
+    Identity.useIdentity(IdentityEnum.ACCOUNT, world.getSaved<string>('accountName') || ZENKO_ACCOUNT_NAME);


you'll see a lot of similar changes like this in the pr :
I'm removing ambiguity as much as possible when it is not used and not useful

instead of updating code all over the place, should we not initialize the world (why drop support for world.parameters) once on startup?

The current world parameters is currently automatically loaded from the --world-parameters json value.
The new type I added loads value with a kubernete client

Do you mean that you would prefer all the parameters staying in the ZenkoWorldParameters ? I think it's possible but may introduce some other complexities (the world is recreated before each scenario in a Before hook, with Zenko.init), + I want the new TestParameters to be usable with any tests suite, and I don't like the --world-parameters so much (only strings allowed, need to define values in bash script) so i want to avoid ultimately have everything in the new TestParameters interface

I mean parameters should be a World -i.e. Zenko-, as it is the standard way to define the environment for cucumber tests.

We may keep parameters in ZenkoWorldParameters, or possibly remove/tweak some; and use the new "config" lib to initialize the Zenko world instance. But IMHO there should not be coupling between tests and the config lib: should be handled through the world.

SylvainSenechal · 2026-05-20T21:28:39Z

        const client = new ScubaClient({
-            port: parseInt(this.parameters.UtilizationServicePort),
-            host: this.parameters.UtilizationServiceHost,
+            port: 80,


could be stored in some UTILIZATION_SERVICE_PORT = 80 const, I did it for a few other variables.
Not super important, its just always 80 defautl http port

SylvainSenechal · 2026-05-20T21:32:35Z

-                accessKeyId: this.parameters.AccountAccessKey,
-                secretAccessKey: this.parameters.AccountSecretKey,
-            });
+        Identity.defaultAccountName = ZENKO_ACCOUNT_NAME;


Side note on this whole long Identity thing part :
I'm thinking this is gonna change again in another pr, I don't find it super clear, lot's of ifs(somethingDefined), very smelly. It works but not very clean

SylvainSenechal · 2026-05-20T21:33:25Z

            break;
        case EntityType.STORAGE_MANAGER:
-            await this.prepareARWWI(this.parameters.StorageManagerUsername || 'storage_manager',
+            await this.prepareARWWI('storage_manager',


I will probably redefine constants later, but same thing : this is also defined in cli-testing, there is some annoying coupling, don't wanna touch it too much if its gonna change again later

SylvainSenechal · 2026-05-20T21:34:47Z

 });

 describe('Backbeat replication metrics data', function dF() {
+    let scalityUtils;


Lots of upcoming pattern like this one, moving top level definition into before hooks :
Can't define these at the top level because the config is not loaded yet at that time (clients are created a in mocha before all setup)

not sure it's better, but could be done if the "config" exported by 'tests_common/configuration' is the result of initConfig (same pattern as cloudserver's Config)

I'm not sure which pattern you are referencing from cloudserver

in cloudserver, config.js it looks like this:

class Config { constructor() { // load config file.... } }; export { config: new Config(), Config, };

so when you import config from cloudserver you immediately get the actual config which was loaded

SylvainSenechal · 2026-05-20T21:37:39Z

this whole file should disappear, and we should just initialize clients from the widely available config.
then tests could simply call config.vaultClient, config.s3Client etc

SylvainSenechal · 2026-05-20T21:38:09Z

this is picked up & run automatically by mocha on startup

SylvainSenechal · 2026-05-20T21:40:02Z

-                    'ascii',
-                );
-            }
+        if (VAULT_ENDPOINT.startsWith('https://')) {


vaultendpoint is hardcoded as http so this is never run 👀

Keeping it for now because I'm thinking at some there was probably a good reason to have these, maybe will remove later, not really the role of this pr

SylvainSenechal · 2026-05-20T21:42:37Z

+    };
+    PRAAdmin?: ServiceUserCredentials;
+    AdminCredentials: ServiceUserCredentials;
+    ZenkoAccount: {


Might deserve some renaming later or re organisation later, or structure/substrucutre, but it will come more naturally when more variables are migrated here

even if it ends up being changed later, "ZenkoAccount" struct is really weird : an s3Client and IamClient are not part of an account ; and credentials is the actual identifiers of the account...
→ better to just remove the nesting?

francoisferrand

Great improvement to move from setup script to TS module;

However lots of the changes seems to be here just for the sake of it: the config object is really just being used as world was, so it would all more naturally fit to split responsibility:

tests_common handles reading all parameters from the environment, (eventually) helpers to setup tunnels and clients...
worldParameters is THE standard way to pass parameters in cucumber.js (and already used), so it should stay : the change should just update these world parameters using tests_common

This would greatly reduce the scope of the change, while separating responsibility (reading env VS adapting config for cucumber).

francoisferrand · 2026-05-21T18:54:20Z

    # --- 13. Kafka host from backbeat config + port-forward ---
    KAFKA_HOST_PORT_ORIG=$(kubectl get secret -l app.kubernetes.io/name=backbeat-config,app.kubernetes.io/instance=${ZENKO_NAME} \
        -o jsonpath='{.items[0].data.config\.json}' | base64 -di | jq -r .kafka.hosts)
    KAFKA_SVC=${KAFKA_HOST_PORT_ORIG%:*}
    KAFKA_PORT=${KAFKA_HOST_PORT_ORIG#*:}

    # Port-forward for Kafka broker (TCP — no ingress possible)
    if ! ss -tlnp 2>/dev/null | grep -q ":${KAFKA_PORT}" && \
       ! lsof -i ":${KAFKA_PORT}" &>/dev/null; then
        kubectl port-forward "svc/${KAFKA_SVC}" "${KAFKA_PORT}:${KAFKA_PORT}" &>/dev/null &
        _KAFKA_PF_PID=$!
        timeout 10 bash -c "until ss -tlnp 2>/dev/null | grep -q ':${KAFKA_PORT}'; do sleep 0.2; done"
    fi
    export KAFKA_HOST_PORT="localhost:${KAFKA_PORT}"


not for this PR, but kafka port forward can be setup from tests as well.

(also, FYI: in Golang you can (in the kafka client) handle the "resolve" part, so you don't need to modify /etc/hosts ; it would be great if we can do the same in node...)

Ok I didn't know we could do that from the tests directly, we can do it later yeah

please create followup ticket

francoisferrand · 2026-05-21T18:55:40Z

@@ -235,20 +197,6 @@
    fi
    export PROMETHEUS_SERVICE="${PROMETHEUS_SVC}.${NAMESPACE}.svc.cluster.local"


could be dynamically retrieved as well?
there is most likely a single prometheus service in the cluster: we could find it by labels ("managed by prometheus operator") or checking the prometheus CR...

There are multiple values we can load that I didn't add yet
My problem is, for PROMETHEUS_SERVICE for example, its built from PROMETHEUS_SVC, but PROMETHEUS_SVC is used right before that to do the port forward. So If I move PROMETHEUS_SERVICE to the ts code, I still need to have it defined in the sh script and then we have the same variable defined in 2 places. Best is to keep it here for now, as you said later we should be able to do the port forward from the node code so we can do it here

please create followup ticket

francoisferrand · 2026-05-21T18:56:42Z

    export AZURE_ARCHIVE_ACCESS_TIER="Hot"
    export AZURE_ARCHIVE_MANIFEST_ACCESS_TIER="Hot"


nothing specific here (I think), should be in tests or read from cluster...

Yeah i can move it now since you mention it, but the reason I didnt do it is just that I didnt fully review the whole file as the pr is getting big

francoisferrand · 2026-05-21T18:58:08Z

    }
-    Identity.useIdentity(IdentityEnum.ACCOUNT, world.getSaved<string>('accountName') ||
-        world.parameters.AccountName);
+    Identity.useIdentity(IdentityEnum.ACCOUNT, world.getSaved<string>('accountName') || ZENKO_ACCOUNT_NAME);


instead of updating code all over the place, should we not initialize the world (why drop support for world.parameters) once on startup?

francoisferrand · 2026-05-21T19:00:08Z

+BeforeAll(async function () {
+    // Some hooks are defined in cli-testing and use the configuration,
+    // we need to have this run before anything else
+    await initConfig();


since we need to init config anyway : best initialize the world, which is the CTST (or even cucumber?) concept to hold such configuration

I explored a bit the best strategy to deal with this config initialization, but I think here is the best place to do it.
The World class is created for each scenario, the constructor of the world can't be asynchronous, so its a bit tricky to do this initialization in the world constructor or as a static config.
BeforeAll is run once per worker, before the world creation, and before pretty much everything since its the first beforeAll, so the config is loaded once and available everywhere

so World constructor will be called after config is init, and can used the config directly?

francoisferrand · 2026-05-21T19:12:28Z

 });

 describe('Backbeat replication metrics data', function dF() {
+    let scalityUtils;


not sure it's better, but could be done if the "config" exported by 'tests_common/configuration' is the result of initConfig (same pattern as cloudserver's Config)

francoisferrand · 2026-05-21T19:19:51Z

+import { NodeHttpHandler } from '@smithy/node-http-handler';
+import { getSecretByLabel, getSecretByName, getCustomObject } from "./kubernetes";
+
+const ZENKO_NAME = 'end2end';


we should keep it.
goal (eventually) is to be able to run against any zenko instance (e.g. artesca), so best to keep it: it does not hurt.

Issue: ZENKO-5259

francoisferrand

as discussed, pending some rework before reviewing

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from b80c535 to 0ddc9e7 Compare April 27, 2026 16:30

claude Bot reviewed Apr 28, 2026

View reviewed changes

Comment thread tests/functional/ctst/common/hooks.ts Outdated

claude Bot reviewed Apr 28, 2026

View reviewed changes

Comment thread tests/functional/ctst/common/hooks.ts Outdated

scality deleted a comment from bert-e Apr 30, 2026

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from c016cb6 to c34e56a Compare April 30, 2026 14:11

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from c34e56a to 7dfe4dc Compare April 30, 2026 14:21

claude Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread tests/functional/mocha/setup.ts Outdated

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch 2 times, most recently from 0ddc9e7 to 6605bd7 Compare April 30, 2026 16:25

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from 6605bd7 to 2264a31 Compare April 30, 2026 16:44

claude Bot reviewed Apr 30, 2026

View reviewed changes

Comment thread tests/functional/tests_common/configuration.ts

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch 2 times, most recently from 1805181 to 7b2e0f5 Compare May 4, 2026 10:21

claude Bot reviewed May 4, 2026

View reviewed changes

Comment thread tests/functional/tests_common/kubernetes.ts Outdated

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch 3 times, most recently from 4c2c959 to a8934b3 Compare May 4, 2026 12:59

claude Bot reviewed May 4, 2026

View reviewed changes

Comment thread tests/functional/tests_common/configuration.ts Outdated

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from a8934b3 to 54cc19b Compare May 4, 2026 14:36

francoisferrand force-pushed the development/2.14 branch from 59d7858 to a856b60 Compare May 4, 2026 16:48

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch 2 times, most recently from f13d91c to 1c39486 Compare May 5, 2026 17:22

SylvainSenechal changed the base branch from development/2.14 to development/2.15 May 5, 2026 17:23

SylvainSenechal added 2 commits May 20, 2026 14:04

use node 24 in codespace

bea5367

Issue: ZENKO-5259

fix lint issue on identity persona

98252c3

Issue: ZENKO-5259

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from 1c39486 to eb86c97 Compare May 20, 2026 14:16

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from eb86c97 to 0ae18c9 Compare May 20, 2026 18:32

claude Bot reviewed May 20, 2026

View reviewed changes

scality deleted a comment from bert-e May 20, 2026

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from 0ae18c9 to b59bdf4 Compare May 20, 2026 21:18

SylvainSenechal marked this pull request as ready for review May 20, 2026 21:18

SylvainSenechal requested review from a team, benzekrimaha and maeldonn May 20, 2026 21:22

SylvainSenechal commented May 20, 2026

View reviewed changes

Comment thread tests/functional/ctst/world/Zenko.ts

SylvainSenechal commented May 20, 2026

View reviewed changes

francoisferrand reviewed May 21, 2026

View reviewed changes

add utility to setup configuration at the beginning of the tests

b5c9722

Issue: ZENKO-5259

SylvainSenechal force-pushed the improvement/ZENKO-5259 branch from b59bdf4 to b5c9722 Compare May 22, 2026 13:26

SylvainSenechal requested a review from francoisferrand May 22, 2026 13:26

SylvainSenechal marked this pull request as draft May 29, 2026 09:56

francoisferrand reviewed May 29, 2026

View reviewed changes

	const ZENKO_NAME = 'end2end';
	const ZENKO_NAME = process.env.ZENKO_NAME \|\| 'end2end';


		. "$_SETUP_DIR/common.sh"

		ZENKO_NAME="${ZENKO_NAME:-end2end}"

		@@ -235,20 +197,6 @@
		fi
		export PROMETHEUS_SERVICE="${PROMETHEUS_SVC}.${NAMESPACE}.svc.cluster.local"

		export AZURE_ARCHIVE_ACCESS_TIER="Hot"
		export AZURE_ARCHIVE_MANIFEST_ACCESS_TIER="Hot"

Conversation

SylvainSenechal commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Apr 24, 2026

Hello sylvainsenechal,

Uh oh!

Uh oh!

Uh oh!

bert-e commented Apr 30, 2026

Waiting for approval

Uh oh!

Uh oh!

bert-e commented Apr 30, 2026

Waiting for approval

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bert-e commented May 5, 2026

Waiting for approval

Uh oh!

claude Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand left a comment

SylvainSenechal commented Apr 24, 2026 •

edited

Loading

SylvainSenechal May 20, 2026 •

edited

Loading

francoisferrand May 26, 2026 •

edited

Loading