Move S3 config from region to onyxia-web - #1082
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR moves S3 configuration from deployment-region data to environment configuration. Bootstrap parses and exposes S3 settings through root context. S3 profile management, authentication, navigation, home-page routing, account tabs, and Helm deployment use the new configuration and optional Onyxia API URL. ChangesS3 configuration centralization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
web/src/core/ports/OnyxiaApi/S3Config.ts (1)
48-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
.strict()onzClaimFilterandzOidcConfigurationShapeto catch typos.
zClaimFilter's two object branches andzOidcConfigurationShapeare all-optional or loosely matched, andz.object()strips unknown keys by default instead of rejecting them. A typo such as a wrong field name is accepted silently and the mistyped value disappears without any validation error.web/scripts/unyamlify-env-local.tsdemonstrates this exact failure mode withissuerURI/clientIDinstead ofissuerUri/clientId.Add
.strict()to these object schemas so unexpected or misspelled keys throw during environment parsing instead of being dropped silently.♻️ Proposed fix
const zClaimFilter = z.union([ - z.object({ - claimName: z.undefined().optional() - }), - z.object({ + z.object({ + claimName: z.undefined().optional() + }).strict(), + z.object({ claimName: z.string(), includedClaimPattern: z.string().optional(), excludedClaimPattern: z.string().optional() - }) + }).strict() ]);const zOidcConfigurationShape = z.object({ issuerUri: z.string().optional(), clientId: z.string().optional(), extraQueryParams_raw: z.string().optional(), scope_spaceSeparated: z.string().optional(), idleSessionLifetimeInSeconds: z.number().optional() - }); + }).strict();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/OnyxiaApi/S3Config.ts` around lines 48 - 69, Apply .strict() to both object branches within zClaimFilter and to zOidcConfigurationShape so unknown or misspelled keys are rejected during validation rather than stripped. Leave the surrounding zOidcConfiguration custom validation and defined fields unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/scripts/unyamlify-env-local.ts`:
- Around line 48-53: Update the OIDC template fields in unyamlify-env-local.ts
to use the S3Config schema names issuerUri and clientId instead of issuerURI and
clientID, preserving the existing issuer and client values.
In `@web/src/core/ports/OnyxiaApi/S3Config.ts`:
- Around line 87-100: Resolve the S3 monitoring configuration mismatch by either
adding the monitoring field consistently to S3Config_UserProvided and
S3Config_Parsed.Entry and propagating it through s3Config_userProvidedToParsed,
or removing S3.monitoring.URLPattern from unyamlify-env-local.ts if unsupported.
Ensure the chosen behavior prevents the field from being silently stripped.
In `@web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts`:
- Around line 188-193: Ensure the S3 profile aggregation flow skips entries
whose resolved STS configuration contains no roles before parsing or asserting
on it. Update the logic around resolvedTemplatedStsRoles and
aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet to require at least one
valid stsRoles entry, removing or excluding unresolved entries while preserving
valid profile aggregation.
---
Nitpick comments:
In `@web/src/core/ports/OnyxiaApi/S3Config.ts`:
- Around line 48-69: Apply .strict() to both object branches within zClaimFilter
and to zOidcConfigurationShape so unknown or misspelled keys are rejected during
validation rather than stripped. Leave the surrounding zOidcConfiguration custom
validation and defined fields unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 74665204-c19a-40dc-96e1-ebe72af5b75f
📒 Files selected for processing (21)
web/.envweb/scripts/unyamlify-env-local.tsweb/src/core/adapters/onyxiaApi/ApiTypes.tsweb/src/core/adapters/onyxiaApi/onyxiaApi.tsweb/src/core/bootstrap.tsweb/src/core/ports/OnyxiaApi/DeploymentRegion.tsweb/src/core/ports/OnyxiaApi/S3Config.tsweb/src/core/rootContext.tsweb/src/core/usecases/s3ExplorerUiController/thunks.tsweb/src/core/usecases/s3ProfilesCreationUiController/thunks.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedBookmark.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.tsweb/src/core/usecases/s3ProfilesManagement/selectors.tsweb/src/core/usecases/s3ProfilesManagement/state.tsweb/src/core/usecases/s3ProfilesManagement/thunks.tsweb/src/env.tsweb/src/ui/App/App.tsxweb/src/ui/App/LeftBar.tsxweb/src/ui/pages/home/Page.tsxweb/src/vite-env.d.ts
💤 Files with no reviewable changes (3)
- web/src/core/adapters/onyxiaApi/ApiTypes.ts
- web/src/core/adapters/onyxiaApi/onyxiaApi.ts
- web/src/core/ports/OnyxiaApi/DeploymentRegion.ts
Comment on lines
+188
to
+193
| if (fromAdminConfig.resolvedTemplatedStsRoles === undefined) { | ||
| return []; | ||
| } | ||
|
|
||
| const entry = fromRegion.resolvedTemplatedStsRoles.find( | ||
| e => e.correspondingS3ConfigIndexInRegion === index | ||
| const entry = fromAdminConfig.resolvedTemplatedStsRoles.find( | ||
| e => e.correspondingS3ConfigEntryIndex === index |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline web/src/core/ports/OnyxiaApi/S3Config.ts --items all
rg -n -C 5 'roles\s*:|\.min\(|nonempty\(' \
web/src/core/ports/OnyxiaApi/S3Config.ts
rg -n -C 4 'roles:\s*\[\s*\]|sts:.*roles' web \
-g '*.{ts,tsx,json,yaml,yml}'Repository: InseeFrLab/onyxia
Length of output: 1687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' web/src/core/ports/OnyxiaApi/S3Config.ts
printf '\n--- s3Profiles aggregate section ---\n'
sed -n '150,220p' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
printf '\n--- schema occurrences ---\n'
rg -n -C 4 'S3Config_UserProvided|s3Config_userProvidedToParsed|zS3Config_Entry|roles|entry\.roles|sts: ' web/src/core/ports/OnyxiaApi/S3Config.ts web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.tsRepository: InseeFrLab/onyxia
Length of output: 17003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- s3Profiles relevant whole function ---'
rg -n -A 220 -B 40 'resolvedTemplatedStsRoles\.map|resolvedTemplatedStsRoles_forThisProfile' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
printf '\n%s\n' '--- imports and surrounding definitions of useS3ProfilesProfileManager ---'
rg -n -C 80 'useS3ProfilesProfileManager|profileName =|resolvedTemplatedStsRoles_forThisProfile|profileName.*fromAdminConfig|correspondingS3ConfigEntryIndex' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
printf '\n%s\n' '--- occurrences of resolvedTemplatedStsRoles in repo ---'
rg -n -C 8 'resolvedTemplatedStsRoles|correspondingS3ConfigEntryIndex' web/src/core/usecases/s3ProfilesManagement -g '*.ts'Repository: InseeFrLab/onyxia
Length of output: 36565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
async function main() {
for await (const module of [
"core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole",
"core/tools/resolveTemplate",
"core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplates",
]) {
try {
const path = `web/src/core/${module}.ts`;
const { createRequire } = await import("module");
const fs = await import("fs");
if (!fs.existsSync(path)) {
console.log(`${module}: missing ${path}`);
continue;
}
console.log(`\n--- ${module} outline ---`);
const outline = await import("ast-grep");
// Skip external dependency inspection when ast-grep Node API is unavailable.
} catch (error) {
console.log(`${module}: unavailable`);
}
const { readFileSync } = await import("fs");
console.log(readFileSync(`web/src/core/${module}.ts`, "utf8"));
}
}
main();
JSRepository: InseeFrLab/onyxia
Length of output: 566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resolveTemplatedStsRole.ts ---'
sed -n '1,260p' web/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole.ts
printf '%s\n' '--- resolveTemplate.ts candidates ---'
fd -a 'resolveTemplate.*\.ts$' web/src/core | sed 's#^\./##' | head -50
printf '%s\n' '--- resolveTemplates.ts candidates ---'
fd -a 'resolveTemplates.*\.ts$' web/src/core | sed 's#^\./##' | head -50
printf '%s\n' '--- imports of resolveTemplatedStsRole ---'
rg -n -C 3 'resolveTemplatedStsRole|resolveTemplate|resolveTemplates' web/src/core -g '*.ts'Repository: InseeFrLab/onyxia
Length of output: 10139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'OnyxiaApi.*\.ts$' web/src/core/ports | sed 's#^\./##' | xargs -r rg -n -C 6 'S3Config_UserProvided|zS3Config_UserProvided|s3Config_userProvidedToParsed|role:'
printf '%s\n' '--- OnyxiaApi exports ---'
fd -a 'OnyxiaApi.*\.ts$' web/src/core/ports | sed 's#^\./##' | xargs -r rg -n -C 5 'export .*S3|S3Config|s3Config'Repository: InseeFrLab/onyxia
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- OnyxiaApi files ---'
git ls-files web/src/core/ports/OnyxiaApi
printf '%s\n' '--- S3 config validation occurrences ---'
rg -n -C 5 'S3Config_UserProvided|zS3Config_UserProvided|s3Config_userProvidedToParsed|role:|min\(|nonempty|atLeast' web/src/core/ports web/src/core/usecases/web -g '*.ts' || true
printf '%s\n' '--- schema field docs around S3 config ---'
rg -n -C 4 'S3Config_UserProvided|zS3Config_UserProvided|role\s*:|sts\.role' web -g '*.ts' -g '*.tsx' || trueRepository: InseeFrLab/onyxia
Length of output: 18220
Handle STS entries that resolve no roles.
resolveTemplatedStsRole can return [] when a claim value is absent, empty, rejected by filters, or no substitutions match. That empty result makes fromAdminConfig.entries[index].resolvedTemplatedStsRoles.stsRoles empty, and aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet asserts before creating a profile. Require at least one valid STS role before parsing, or remove entries that resolve no roles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts`
around lines 188 - 193, Ensure the S3 profile aggregation flow skips entries
whose resolved STS configuration contains no roles before parsing or asserting
on it. Update the logic around resolvedTemplatedStsRoles and
aggregateS3ProfilesFromVaultAndRegionIntoAnUnifiedSet to require at least one
valid stsRoles entry, removing or excluding unresolved entries while preserving
valid profile aggregation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/core/ports/OnyxiaApi/S3Config.ts`:
- Line 65: Update the idleSessionLifetimeInSeconds schema and its conversion
logic near the existing parsing code to accept numbers or non-empty strings
containing only decimal integer digits. Reject values such as "3600s" and
"not-a-number" before conversion, while preserving valid numeric values and
integer strings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1db3358a-e9c6-477c-952c-2a16144fdea6
📒 Files selected for processing (2)
web/scripts/unyamlify-env-local.tsweb/src/core/ports/OnyxiaApi/S3Config.ts
💤 Files with no reviewable changes (1)
- web/scripts/unyamlify-env-local.ts
| clientID: z.string().optional(), | ||
| extraQueryParams: z.string().optional(), | ||
| scope: z.string().optional(), | ||
| idleSessionLifetimeInSeconds: z.union([z.number(), z.string()]).optional() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject malformed idle-session duration strings.
z.string() accepts "3600s" and "not-a-number". Line 225 truncates the first value and produces NaN for the second value. Restrict non-empty strings to decimal integers before conversion.
Proposed fix
- idleSessionLifetimeInSeconds: z.union([z.number(), z.string()]).optional()
+ idleSessionLifetimeInSeconds: z
+ .union([z.number(), z.literal(""), z.string().regex(/^\d+$/)])
+ .optional()
@@
- return parseInt(value);
+ return Number(value);Also applies to: 213-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/ports/OnyxiaApi/S3Config.ts` at line 65, Update the
idleSessionLifetimeInSeconds schema and its conversion logic near the existing
parsing code to accept numbers or non-empty strings containing only decimal
integer digits. Reject values such as "3600s" and "not-a-number" before
conversion, while preserving valid numeric values and integer strings.
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/core/ports/OnyxiaApi/S3Config.ts (1)
50-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate claim-filter regular-expression patterns during S3 config parsing.
includedClaimPatternandexcludedClaimPatternare accepted as plain strings now.resolveTemplatedStsRoleandresolveTemplatedBookmarkcompile them withnew RegExp(...)later. A malformed pattern lets the S3 env parse successfully and fails only during profile resolution. Reject invalid patterns inzClaimFilter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/OnyxiaApi/S3Config.ts` around lines 50 - 58, Update zClaimFilter to validate includedClaimPattern and excludedClaimPattern as regular-expression patterns during S3 configuration parsing, while preserving their optional string behavior for valid values. Ensure malformed patterns are rejected before resolveTemplatedStsRole or resolveTemplatedBookmark compiles them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/core/adapters/onyxiaApi/mock.ts`:
- Around line 18-31: Update the standalone API mock’s user and project fixtures
in userAndProjects to provide minimal safe placeholder values for the user
fields consumed by bootstrapCore and launch (familyName, firstName, username,
and email) and the project fields consumed after login (name and namespace),
while keeping unrelated guarded fields protected.
In `@web/src/ui/pages/account/Page.tsx`:
- Around line 52-58: Update the account page route handling so when
ONYXIA_API_URL is undefined, unavailable route.params.tabId values are
normalized to "user-interface" or rejected before activeTabId and the content
switch use them; keep available tabs unchanged. Add a regression test covering a
direct unavailable-tab URL in standalone mode.
In `@web/src/ui/pages/home/Page.tsx`:
- Around line 25-28: Update the homepage fallback logic around the
DISABLE_HOMEPAGE and ONYXIA_API_URL checks so it matches LeftBar’s feature
availability: when ONYXIA_API_URL is undefined, avoid redirecting to
routes.catalog() and only redirect to routes.s3Explorer_root() when S3 Explorer
is actually disabled, preserving S5 Explorer as the available no-API fallback.
- Around line 25-29: Move the environment checks currently inside the useConst
initializer in Home into the Home component body so they return from Home after
scheduling the route replacement. Preserve the existing redirects for missing
configuration, remove the no-op return after routes.s3Explorer_root().replace(),
and ensure remaining hooks and JSX are not executed when either check matches.
---
Outside diff comments:
In `@web/src/core/ports/OnyxiaApi/S3Config.ts`:
- Around line 50-58: Update zClaimFilter to validate includedClaimPattern and
excludedClaimPattern as regular-expression patterns during S3 configuration
parsing, while preserving their optional string behavior for valid values.
Ensure malformed patterns are rejected before resolveTemplatedStsRole or
resolveTemplatedBookmark compiles them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c55a8a45-b5f1-4cb4-9b7e-d015d1736c6c
📒 Files selected for processing (19)
web/.envweb/src/core/adapters/onyxiaApi/mock.tsweb/src/core/bootstrap.tsweb/src/core/ports/OnyxiaApi/S3Config.tsweb/src/core/usecases/launcher/thunks.tsweb/src/core/usecases/s3ExplorerUiController/thunks.tsweb/src/core/usecases/s3ProfilesCreationUiController/thunks.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedBookmark.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedStsRole.tsweb/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.tsweb/src/core/usecases/s3ProfilesManagement/selectors.tsweb/src/core/usecases/s3ProfilesManagement/thunks.tsweb/src/core/usecases/userAuthentication/thunks.tsweb/src/env.tsweb/src/ui/App/App.tsxweb/src/ui/App/LeftBar.tsxweb/src/ui/pages/account/Page.tsxweb/src/ui/pages/home/Page.tsxweb/src/vite-env.d.ts
💤 Files with no reviewable changes (1)
- web/src/core/usecases/userAuthentication/thunks.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- web/src/vite-env.d.ts
- web/src/core/usecases/s3ProfilesCreationUiController/thunks.ts
- web/src/core/usecases/s3ProfilesManagement/decoupledLogic/resolveTemplatedBookmark.ts
- web/src/core/usecases/s3ProfilesManagement/decoupledLogic/s3Profiles.ts
- web/src/core/usecases/s3ProfilesManagement/thunks.ts
- web/src/ui/App/App.tsx
- web/src/core/usecases/s3ExplorerUiController/thunks.ts
- web/src/core/usecases/s3ProfilesManagement/selectors.ts
garronej
force-pushed
the
s3_explorer_standalone
branch
from
August 7, 2026 17:45
0754d05 to
f6ab37c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm-chart/templates/web/deployment.yaml`:
- Around line 3-8: Make the first-region S3 selection explicit in the
regions-to-web environment logic: document or otherwise define
api.regions[0].data.S3 as the authoritative global S3 configuration, and add
explicit conflict handling if other regions also provide differing S3 values.
Preserve the existing web.env.S3 assignment for the authoritative configuration.
- Around line 49-56: Keep the API context path consistent across consumers by
deriving the default ONYXIA_API_URL in helm-chart/templates/web/deployment.yaml
(lines 49-56), the HTTPRoute match in helm-chart/templates/httproute.yaml (lines
55-63), and the Ingress path in helm-chart/templates/ingress.yaml (lines 50-58)
from .Values.api.contextPath instead of hardcoding /api.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91f034ad-d3b9-40cb-a42f-50f926929a9c
📒 Files selected for processing (20)
helm-chart/README.mdhelm-chart/templates/api/cluster-role-binding.yamlhelm-chart/templates/api/configmap.yamlhelm-chart/templates/api/configmaps-schemas.yamlhelm-chart/templates/api/configmaps-userprofile.yamlhelm-chart/templates/api/deployment.yamlhelm-chart/templates/api/rolebinding-namespace-admin.yamlhelm-chart/templates/api/route.yamlhelm-chart/templates/api/service.yamlhelm-chart/templates/api/serviceaccount-api.yamlhelm-chart/templates/httproute.yamlhelm-chart/templates/ingress.yamlhelm-chart/templates/tests/test-connection-api.yamlhelm-chart/templates/web/deployment.yamlhelm-chart/values.yamlweb/.envweb/src/core/adapters/onyxiaApi/mock.tsweb/src/core/ports/OnyxiaApi/S3Config.tsweb/src/ui/pages/account/Page.tsxweb/src/ui/pages/account/route.ts
💤 Files with no reviewable changes (1)
- web/.env
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/core/adapters/onyxiaApi/mock.ts
- web/src/ui/pages/account/Page.tsx
- web/src/core/ports/OnyxiaApi/S3Config.ts
Comment on lines
+49
to
56
| {{- if and .Values.api.enabled (not (hasKey $webEnv "ONYXIA_API_URL")) }} | ||
| - name: ONYXIA_API_URL | ||
| value: "/api" | ||
| {{- end }} | ||
| {{- range $key, $value := $webEnv }} | ||
| - name: {{ $key }} | ||
| value: {{ $value | quote }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the API context path consistent across all consumers.
api.contextPath is applied to the API, but the web default and both public routes hardcode /api. A non-default context path makes the frontend and proxies address an API path that is not served. Derive all three values from .Values.api.contextPath, or reject non-/api values. (raw.githubusercontent.com)
helm-chart/templates/web/deployment.yaml#L49-L56: use.Values.api.contextPathfor the defaultONYXIA_API_URL.helm-chart/templates/httproute.yaml#L55-L63: use.Values.api.contextPathfor the HTTPRoute match.helm-chart/templates/ingress.yaml#L50-L58: use.Values.api.contextPathfor the Ingress path.
Proposed alignment
- value: "/api"
+ value: {{ .Values.api.contextPath | quote }}
- value: /api
+ value: {{ .Values.api.contextPath | quote }}
- - path: /api
+ - path: {{ .Values.api.contextPath | quote }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if and .Values.api.enabled (not (hasKey $webEnv "ONYXIA_API_URL")) }} | |
| - name: ONYXIA_API_URL | |
| value: "/api" | |
| {{- end }} | |
| {{- range $key, $value := $webEnv }} | |
| - name: {{ $key }} | |
| value: {{ $value | quote }} | |
| {{- end }} | |
| {{- if and .Values.api.enabled (not (hasKey $webEnv "ONYXIA_API_URL")) }} | |
| - name: ONYXIA_API_URL | |
| value: {{ .Values.api.contextPath | quote }} | |
| {{- end }} | |
| {{- range $key, $value := $webEnv }} | |
| - name: {{ $key }} | |
| value: {{ $value | quote }} | |
| {{- end }} |
Suggested change
| {{- if and .Values.api.enabled (not (hasKey $webEnv "ONYXIA_API_URL")) }} | |
| - name: ONYXIA_API_URL | |
| value: "/api" | |
| {{- end }} | |
| {{- range $key, $value := $webEnv }} | |
| - name: {{ $key }} | |
| value: {{ $value | quote }} | |
| {{- end }} | |
| {{- if $apiEnabled }} | |
| - matches: | |
| - path: | |
| type: PathPrefix | |
| value: {{ .Values.api.contextPath | quote }} | |
| backendRefs: | |
| - name: {{ $fullNameApi }} | |
| port: {{ $svcPortApi }} | |
| {{- end }} |
Suggested change
| {{- if and .Values.api.enabled (not (hasKey $webEnv "ONYXIA_API_URL")) }} | |
| - name: ONYXIA_API_URL | |
| value: "/api" | |
| {{- end }} | |
| {{- range $key, $value := $webEnv }} | |
| - name: {{ $key }} | |
| value: {{ $value | quote }} | |
| {{- end }} | |
| {{- if $apiEnabled }} | |
| - path: {{ .Values.api.contextPath | quote }} | |
| pathType: Prefix | |
| backend: | |
| service: | |
| name: {{ $fullNameApi }} | |
| port: | |
| number: {{ $svcPortApi }} | |
| {{- end }} |
📍 Affects 3 files
helm-chart/templates/web/deployment.yaml#L49-L56(this comment)helm-chart/templates/httproute.yaml#L55-L63helm-chart/templates/ingress.yaml#L50-L58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm-chart/templates/web/deployment.yaml` around lines 49 - 56, Keep the API
context path consistent across consumers by deriving the default ONYXIA_API_URL
in helm-chart/templates/web/deployment.yaml (lines 49-56), the HTTPRoute match
in helm-chart/templates/httproute.yaml (lines 55-63), and the Ingress path in
helm-chart/templates/ingress.yaml (lines 50-58) from .Values.api.contextPath
instead of hardcoding /api.
…I_URL with the helm chart
garronej
force-pushed
the
s3_explorer_standalone
branch
from
August 10, 2026 15:34
b7e8c52 to
2286080
Compare
garronej
force-pushed
the
s3_explorer_standalone
branch
from
August 10, 2026 16:08
3f8529d to
0ad23b3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation