Skip to content

WIP: Migrating from bespoke 's3Configs' that where assotiated to a wo… #1038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
garronej wants to merge 1 commit into from
+577 −956
Closed

WIP: Migrating from bespoke 's3Configs' that where assotiated to a wo… #1038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions web/src/core/adapters/s3Client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,57 @@
export * from "./s3Client";

const x = {
workingDirectory: {
bucketMode: "multi",
bucketNamePrefix: "",
bucketNamePrefixGroup: "projet-"
},
bookmarkedDirectories: [
{
fullPath: "donnees-insee/diffusion/",
title: {
fr: "Données de diffusion",
en: "Dissemination Data"
},
description: {
fr: "Bucket public destiné à la diffusion de données",
en: "Public bucket intended for data dissemination"
}
}
]
};
Comment on lines +3 to +22
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Remove or export the unused constant.

The constant x is defined but not exported, making it inaccessible to other modules. The non-descriptive name and lack of usage suggest this is development scaffolding that should be removed.

Run the following script to verify this constant is not referenced elsewhere:

🏁 Script executed:

#!/bin/bash
# Description: Search for any references to the constant 'x' from this module.

# Search for imports or usage of this constant
rg -n --type=ts --type=tsx 'from.*s3Client.*x|s3Client.*\.x' -g '!web/src/core/adapters/s3Client/index.ts'

Length of output: 27

Run the following script to verify there are no references to the constant x:

🏁 Script executed:

#!/bin/bash
# Search for named imports of `x` from any module
rg -nP -- 'import\s+\{[^}]*\bx\b[^}]*\}' -g '*.ts' -g '*.tsx'
# Search for any property access on a variable named `x`
rg -nP -- '\bx\.' -g '*.ts' -g '*.tsx'

Length of output: 353

Run the following script to verify there are no references to the constant x:

🏁 Script executed:

#!/bin/bash
# Search for named imports of `x` in TS/TSX files
rg -n --glob '*.ts' --glob '*.tsx' 'import\s+{[^}]*\bx\b[^}]*}' .
# Search for any direct property access on `x`
rg -n --glob '*.ts' --glob '*.tsx' '\bx\.' .

Length of output: 256

Remove unused constant x
The constant x is defined but never exported or referenced; remove it or rename and export it if it’s required.

🤖 Prompt for AI Agents 
In web/src/core/adapters/s3Client/index.ts around lines 3 to 22, there is an
unused constant named `x`; remove the dead constant declaration to eliminate
unused code (or if intended to be used, rename it to a descriptive identifier
and export or reference it where needed). Ensure imports/exports and linting
pass after removing or exporting/renaming the constant.

const s3 = {
bookmarkedDirectories: [
{
fullPath: "$1/",
title: "Personal",
description: "Personal storage",
claimName: "preferred_username"
},
{
fullPath: "projet-$1/",
title: "Group $1",
description: "Shared storage for project $1",
claimName: "groups",
excludedClaimPattern: "^USER_ONYXIA$"
},
{
fullPath: "donnees-insee/diffusion/",
title: {
fr: "Données de diffusion",
en: "Dissemination Data"
},
description: {
fr: "Bucket public destiné à la diffusion de données",
en: "Public bucket intended for data dissemination"
}
}
]
};
Comment on lines +23 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Remove or properly utilize the unused constant.

The constant s3 is defined but not exported, rendering it inaccessible. While the structure appears to be a valid S3 profile configuration template (with claim-based path templating), it serves no purpose in its current form.

Consider one of the following actions:

  • Remove if this is leftover scaffolding
  • Export if it's meant to be a reference example
  • Move to documentation if it's illustrative

Run the following script to verify this constant is not referenced elsewhere:

🏁 Script executed:

#!/bin/bash
# Description: Search for any references to the constant 's3' from this module.

# Search for imports or usage of this constant
rg -n --type=ts --type=tsx 'from.*s3Client.*\bs3\b|s3Client\.\bs3\b' -g '!web/src/core/adapters/s3Client/index.ts'

Length of output: 27

🏁 Script executed:

#!/bin/bash
# Description: Search for any references to the constant 's3' from this module.

# Search for import statements or usage of s3Client across TS and TSX files
rg -n "s3Client" -g "*.ts" -g "*.tsx" -g "!web/src/core/adapters/s3Client/index.ts"

Length of output: 6225

Remove or export the unused s3 constant.

The s3 object is declared but never exported or referenced within the codebase, resulting in dead code. Either delete it if it’s leftover scaffolding, or export it (or relocate it to documentation) if it’s intended for reuse.

🤖 Prompt for AI Agents 
In web/src/core/adapters/s3Client/index.ts around lines 23 to 50 there is an
unused constant s3 that creates bookmarkedDirectories but is neither exported
nor referenced; remove the dead constant if it’s leftover scaffolding, or
alternatively export it (e.g., export const s3 = ...) or move it into
documentation/config where intended for reuse so the symbol is either used or
intentionally exposed.


/*
*/
46 changes: 0 additions & 46 deletions web/src/core/adapters/s3Client/s3Client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export namespace ParamsOfCreateS3Client {
roleSessionName: string;
}
| undefined;
nameOfBucketToCreateIfNotExist: string | undefined;
};
}

Expand Down Expand Up @@ -251,51 +250,6 @@ export function createS3Client(
return { getAwsS3Client };
})();

create_bucket: {
if (!params.isStsEnabled) {
break create_bucket;
}

const { nameOfBucketToCreateIfNotExist } = params;

if (nameOfBucketToCreateIfNotExist === undefined) {
break create_bucket;
}

const { awsS3Client } = await getAwsS3Client();

const { CreateBucketCommand, BucketAlreadyExists, BucketAlreadyOwnedByYou } =
await import("@aws-sdk/client-s3");

try {
await awsS3Client.send(
new CreateBucketCommand({
Bucket: nameOfBucketToCreateIfNotExist
})
);
} catch (error) {
assert(is<Error>(error));

if (
!(error instanceof BucketAlreadyExists) &&
!(error instanceof BucketAlreadyOwnedByYou)
) {
console.log(
"An unexpected error occurred while creating the bucket, we ignore it:",
error
);
break create_bucket;
}

console.log(
[
`The above network error is expected we tried creating the `,
`bucket ${nameOfBucketToCreateIfNotExist} in case it didn't exist but it did.`
].join(" ")
);
}
}

return { getNewlyRequestedOrCachedToken, clearCachedToken, getAwsS3Client };
})();

Expand Down
58 changes: 20 additions & 38 deletions web/src/core/ports/OnyxiaApi/DeploymentRegion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ export type DeploymentRegion = {
initScriptUrl: string;
s3Configs: DeploymentRegion.S3Config[];
s3ConfigCreationFormDefaults:
| (Pick<DeploymentRegion.S3Config, "url" | "pathStyleAccess" | "region"> & {
workingDirectory: DeploymentRegion.S3Config["workingDirectory"] | undefined;
})
| Pick<DeploymentRegion.S3Config, "url" | "pathStyleAccess" | "region">
| undefined;
allowedURIPatternForUserDefinedInitScript: string;
kafka:
Expand Down Expand Up @@ -121,43 +119,27 @@ export namespace DeploymentRegion {
| undefined;
oidcParams: OidcParams_Partial;
};
workingDirectory:
| {
bucketMode: "shared";
bucketName: string;
prefix: string;
prefixGroup: string;
}
| {
bucketMode: "multi";
bucketNamePrefix: string;
bucketNamePrefixGroup: string;
};
bookmarkedDirectories: S3Config.BookmarkedDirectory[];
bookmarks: S3Config.Bookmark[];
};

export namespace S3Config {
export type BookmarkedDirectory =
| BookmarkedDirectory.Static
| BookmarkedDirectory.Dynamic;

export namespace BookmarkedDirectory {
export type Common = {
fullPath: string;
title: LocalizedString;
description: LocalizedString | undefined;
tags: LocalizedString[] | undefined;
};

export type Static = Common & {
claimName: undefined;
};

export type Dynamic = Common & {
claimName: string;
includedClaimPattern: string | undefined;
excludedClaimPattern: string | undefined;
};
}
export type Bookmark = {
bucket: string;
keyPrefix: string;
title: LocalizedString;
description: LocalizedString;
tags: LocalizedString[];
} & (
| {
claimName: undefined;
includedClaimPattern?: never;
excludedClaimPattern?: never;
}
| {
claimName: string;
includedClaimPattern: string | undefined;
excludedClaimPattern: string | undefined;
}
);
}
}
2 changes: 0 additions & 2 deletions web/src/core/usecases/projectManagement/decoupledLogic/ProjectConfigs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ export namespace ProjectConfigs {
friendlyName: string;
url: string;
region: string | undefined;
workingDirectoryPath: string;
pathStyleAccess: boolean;
credentials:
| {
Expand Down Expand Up @@ -108,7 +107,6 @@ const zS3Config = (() => {
friendlyName: z.string(),
url: z.string(),
region: z.union([z.string(), z.undefined()]),
workingDirectoryPath: z.string(),
pathStyleAccess: z.boolean(),
credentials: z.union([zS3Credentials, z.undefined()])
});
Expand Down
22 changes: 7 additions & 15 deletions web/src/core/usecases/s3ConfigConnectionTest/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,10 @@ type State = {

export type OngoingConfigTest = {
paramsOfCreateS3Client: ParamsOfCreateS3Client;
workingDirectoryPath: string;
};

export type ConfigTestResult = {
paramsOfCreateS3Client: ParamsOfCreateS3Client;
workingDirectoryPath: string;
result:
| {
isSuccess: true;
Expand Down Expand Up @@ -43,20 +41,17 @@ export const { actions, reducer } = createUsecaseActions({
payload: State["ongoingConfigTests"][number];
}
) => {
const { paramsOfCreateS3Client, workingDirectoryPath } = payload;
const { paramsOfCreateS3Client } = payload;

if (
state.ongoingConfigTests.find(e =>
same(e, { paramsOfCreateS3Client, workingDirectoryPath })
same(e, { paramsOfCreateS3Client })
) !== undefined
) {
return;
}

state.ongoingConfigTests.push({
paramsOfCreateS3Client,
workingDirectoryPath
});
state.ongoingConfigTests.push({ paramsOfCreateS3Client });
},
Comment on lines +44 to 55
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Keying by paramsOfCreateS3Client only — LGTM.

Deep-compare usage is correct; dedupe and replace semantics look right.

Run to spot stale references to workingDirectoryPath:

Also applies to: 64-85

🏁 Script executed:

#!/usr/bin/env bash
rg -n --type=ts --type=tsx -C2 '\bworkingDirectoryPath\b'

Length of output: 27

🏁 Script executed:

#!/usr/bin/env bash
rg -n '\bworkingDirectoryPath\b' -C2 .

Length of output: 22376

Remove stale workingDirectoryPath references across S3 config usecases
Residual workingDirectoryPath usages in web/src/core/usecases/s3ConfigCreation/thunks.ts, state.ts and selectors.ts must be updated or removed to align with the new dedupe logic keyed solely by paramsOfCreateS3Client.

🤖 Prompt for AI Agents 
In web/src/core/usecases/s3ConfigConnectionTest/state.ts around lines 44-55
there are stale references to workingDirectoryPath in the dedupe logic; remove
any usage of workingDirectoryPath and ensure deduplication and state entries are
keyed only by paramsOfCreateS3Client (i.e., replace same(e, {
paramsOfCreateS3Client, workingDirectoryPath }) checks with comparisons that
only compare paramsOfCreateS3Client, update the object pushed into
state.ongoingConfigTests to contain only { paramsOfCreateS3Client }, and adjust
any related equality helpers or type definitions so the state shape and
comparisons no longer expect or reference workingDirectoryPath).

testCompleted: (
state,
Expand All @@ -66,11 +61,11 @@ export const { actions, reducer } = createUsecaseActions({
payload: State["configTestResults"][number];
}
) => {
const { paramsOfCreateS3Client, workingDirectoryPath, result } = payload;
const { paramsOfCreateS3Client, result } = payload;

remove_from_ongoing: {
const entry = state.ongoingConfigTests.find(e =>
same(e, { paramsOfCreateS3Client, workingDirectoryPath })
same(e, { paramsOfCreateS3Client })
);

if (entry === undefined) {
Expand All @@ -84,10 +79,8 @@ export const { actions, reducer } = createUsecaseActions({
}

remove_existing_result: {
const entry = state.configTestResults.find(
e =>
same(e.paramsOfCreateS3Client, paramsOfCreateS3Client) &&
e.workingDirectoryPath === workingDirectoryPath
const entry = state.configTestResults.find(e =>
same(e.paramsOfCreateS3Client, paramsOfCreateS3Client)
);

if (entry === undefined) {
Expand All @@ -99,7 +92,6 @@ export const { actions, reducer } = createUsecaseActions({

state.configTestResults.push({
paramsOfCreateS3Client,
workingDirectoryPath,
result
});
}
Expand Down
17 changes: 5 additions & 12 deletions web/src/core/usecases/s3ConfigConnectionTest/thunks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,13 @@ export const thunks = {} satisfies Thunks;

export const protectedThunks = {
testS3Connection:
(params: {
paramsOfCreateS3Client: ParamsOfCreateS3Client.NoSts;
workingDirectoryPath: string;
}) =>
(params: { paramsOfCreateS3Client: ParamsOfCreateS3Client.NoSts }) =>
async (...args) => {
const { paramsOfCreateS3Client, workingDirectoryPath } = params;
const { paramsOfCreateS3Client } = params;

const [dispatch] = args;

dispatch(
actions.testStarted({ paramsOfCreateS3Client, workingDirectoryPath })
);
dispatch(actions.testStarted({ paramsOfCreateS3Client }));

const result = await (async () => {
const { createS3Client } = await import("core/adapters/s3Client");
Expand All @@ -31,9 +26,8 @@ export const protectedThunks = {
const s3Client = createS3Client(paramsOfCreateS3Client, getOidc);

try {
await s3Client.listObjects({
path: workingDirectoryPath
});
console.log("Find a way to test only s3 credential", s3Client);
throw new Error("TODO: Not implemented yet");
} catch (error) {
return {
isSuccess: false as const,
Expand All @@ -47,7 +41,6 @@ export const protectedThunks = {
dispatch(
actions.testCompleted({
paramsOfCreateS3Client,
workingDirectoryPath,
result
})
);
Expand Down
Loading