Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved credential exposure, configuration loss, and upgrade-mode correctness issues block approval.
6 open issues
What changed in this PR
Adds an existing-service upgrade workflow through the launcher, using the endpoint proposed in onyxia-api PR #685.
Changes:
- Adds Modify actions and preserves the target release in upgrade mode.
- Prefills configuration using schema-aware value conversion and submits upgrades.
- Adds translations and conversion tests.
| File | Description |
|---|---|
| web/src/ui/pages/myServices/Page.tsx | Opens upgrade mode from service cards. |
| web/src/ui/pages/myServices/MyServicesCards/MyServicesCards.tsx | Passes upgrade callbacks to cards. |
| web/src/ui/pages/myServices/MyServicesCards/MyServicesCard/MyServicesCard.tsx | Adds the Modify action. |
| web/src/ui/pages/myService/Page.tsx | Opens upgrade mode from service details. |
| web/src/ui/pages/myService/MyServiceButtonBar.tsx | Adds the Modify button. |
| web/src/ui/pages/myService/MyServiceButtonBar.stories.tsx | Updates story props. |
| web/src/ui/pages/launcher/route.ts | Adds the upgrade parameter. |
| web/src/ui/pages/launcher/Page.tsx | Connects upgrade routing and state. |
| web/src/ui/pages/launcher/LauncherMainCard.tsx | Displays the Upgrade label. |
| web/src/ui/i18n/resources/zh-CN.tsx | Adds Chinese translations. |
| web/src/ui/i18n/resources/no.tsx | Adds Norwegian translations. |
| web/src/ui/i18n/resources/nl.tsx | Adds Dutch translations. |
| web/src/ui/i18n/resources/it.tsx | Adds Italian translations. |
| web/src/ui/i18n/resources/fr.tsx | Adds French translations. |
| web/src/ui/i18n/resources/fi.tsx | Adds Finnish translations. |
| web/src/ui/i18n/resources/es.tsx | Adds Spanish translations. |
| web/src/ui/i18n/resources/en.tsx | Adds English labels. |
| web/src/ui/i18n/resources/de.tsx | Adds German translations. |
| web/src/core/usecases/serviceManagement/selectors.ts | Exposes release configuration for modification. |
| web/src/core/usecases/serviceDetails/thunks.ts | Loads chart metadata. |
| web/src/core/usecases/serviceDetails/state.ts | Stores upgrade-related metadata. |
| web/src/core/usecases/serviceDetails/selectors.ts | Exposes metadata to the details page. |
| web/src/core/usecases/launcher/thunks.ts | Initializes upgrades and selects the API operation. |
| web/src/core/usecases/launcher/state.ts | Stores the pinned release name. |
| web/src/core/usecases/launcher/selectors.ts | Uses and exposes the pinned name. |
| web/src/core/usecases/launcher/decoupledLogic/index.ts | Exports value coercion. |
| web/src/core/usecases/launcher/decoupledLogic/coerceHelmValuesPatch.ts | Converts values using schema types. |
| web/src/core/usecases/launcher/decoupledLogic/coerceHelmValuesPatch.test.ts | Tests value conversion. |
| web/src/core/tools/flatHelmValuesToPatch.ts | Converts flattened values into patches. |
| web/src/core/ports/OnyxiaApi/OnyxiaApi.ts | Defines the upgrade operation. |
| web/src/core/adapters/onyxiaApi/onyxiaApi.ts | Calls the upgrade endpoint. |
| web/src/core/adapters/onyxiaApi/mock.ts | Adds the upgrade mock placeholder. |
Review effort: Balanced
| return Object.entries(flatHelmValues).map(([key, value]) => ({ | ||
| path: key.split("."), | ||
| value | ||
| })); |
| version: service.chartVersion, | ||
| name: service.friendlyName, | ||
| shared: service.ownership.isShared, | ||
| helmValuesPatch: flatHelmValuesToPatch(service.helmValues), |
| node = typeof segment === "number" ? node?.items : node?.properties?.[segment]; | ||
| } | ||
|
|
||
| return node?.type; |
| // NOTE: In "upgrade" mode the release name is fixed to the existing one. | ||
| return ( | ||
| state.helmReleaseName_pinned ?? `${state.chartName}-${state.k8sRandomSubdomain}` | ||
| ); |
| helmValuesPatch: coerceHelmValuesPatch({ | ||
| helmValuesPatch: helmValuesPatch ?? [], | ||
| helmValuesSchema | ||
| }) |
| isShared: undefined, | ||
| s3ProfileName: undefined | ||
| s3ProfileName: undefined, | ||
| helmReleaseName: undefined |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8df4dd3d0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * | ||
| * NOTE: Array values are not recoverable — the API's flattening drops them | ||
| * (a JSON array node has no object fields to flatten). Such values simply | ||
| * won't be pre-filled and will fall back to the chart defaults. |
There was a problem hiding this comment.
For any service with a user-supplied array, the API flattening emits no patch entry here; launcher initialization fills that path from the chart defaults, and helmUpgrade then sends the entire computed values object. An otherwise no-op Upgrade therefore silently replaces custom arrays with defaults. The flow needs structured current values (or must block the upgrade) rather than treating this omission as safe.
Useful? React with 👍 / 👎.
| s3ProfileName: route.params.s3, | ||
| helmValuesPatch: route.params.helmValuesPatch | ||
| helmValuesPatch: route.params.helmValuesPatch, | ||
| helmReleaseName: route.params.upgrade |
There was a problem hiding this comment.
On deployments where external auto-launch is enabled, opening a URL containing both upgrade=<existing release> and autoLaunch=true reaches this assignment with autoLaunch still true; initialize immediately dispatches launch, whose new branch calls helmUpgrade. An authenticated user can therefore modify a service merely by following a crafted or copied link, so upgrade mode should suppress auto-launch or require an explicit confirmation.
Useful? React with 👍 / 👎.
| isShared: undefined, | ||
| s3ProfileName: undefined | ||
| s3ProfileName: undefined, | ||
| helmReleaseName: undefined |
There was a problem hiding this comment.
While modifying an existing service, selecting Restore all default executes this branch and clears the pinned target. The release-name selector then generates a random name and launch() invokes helmInstall, so an attempt to reset the existing service instead creates a duplicate service. Preserve the pinned release name while resetting only the configuration fields.
Useful? React with 👍 / 👎.
| areHelmValuesShown={isCommandBarExpanded ?? false} | ||
| monitoringUrl={monitoringUrl} | ||
| onClickModify={ | ||
| isReady |
There was a problem hiding this comment.
In group projects, serviceManagement.services includes shared releases owned by another user, and the new card action is explicitly gated by service.ownership.isOwned; this detail screen instead enables Modify solely on isReady. Following the title of a shared non-owned service therefore bypasses that ownership restriction and builds an upgrade request for it. Carry the ownership flag into serviceDetails and apply the same gate.
Useful? React with 👍 / 👎.
| node = typeof segment === "number" ? node?.items : node?.properties?.[segment]; | ||
| } | ||
|
|
||
| return node?.type; |
There was a problem hiding this comment.
The accepted JSON schema contract permits a node with const and no type, and the existing shared getJSONSchemaType helper already infers its type from that constant. This lookup instead returns undefined, so the fallback converts current string constants such as "false" or "007" to booleans or numbers; applying that patch overwrites the valid constant with the wrong type and leaves the upgrade form invalid. Reuse the same const-aware inference here.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ec030ce64
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| value: StringifyableAtomic | undefined; | ||
| }[] { | ||
| return Object.entries(flatHelmValues).map(([key, value]) => ({ | ||
| path: key.split("."), |
There was a problem hiding this comment.
Whenever an option map has a literal dotted key (for example the annotation key prometheus.io/scrape), the flattened input uses dots both as path separators and as key content. This unconditional split reconstructs it as prometheus: { "io/scrape": ... }; initialization applies that different path and helmUpgrade submits the full malformed/defaulted values object, so even a no-op upgrade changes the release. Preserve or resolve escaped segments, or reject upgrades when the flat representation is ambiguous.
Useful? React with 👍 / 👎.
| default: | ||
| if (typeof value !== "string") { | ||
| return value; | ||
| } | ||
| return parseBoolean(value) ?? parseNumber(value) ?? value; |
There was a problem hiding this comment.
When a chart has no values schema (the supported fallback in thunks.initialize) or a value lives under additionalProperties, schema lookup returns undefined and this fallback converts current API strings such as "false" or "007" into booleans or numbers. Those may be intentional string options, especially in environment maps, and helmUpgrade sends the full values object, silently changing behavior on an otherwise no-op upgrade. Infer from values.yaml or structured current values, preserve ambiguous strings, or block the upgrade.
Useful? React with 👍 / 👎.
| return `${state.chartName}-${state.k8sRandomSubdomain}`; | ||
| // NOTE: In "upgrade" mode the release name is fixed to the existing one. | ||
| return ( | ||
| state.helmReleaseName_pinned ?? `${state.chartName}-${state.k8sRandomSubdomain}` |
There was a problem hiding this comment.
In the Launcher Page command-bar/download path (Page.tsx uses launchScript), upgrade mode now supplies the existing release name here, but launchCommands still hard-codes helm install. The Helm reference calls helm install [NAME] [CHART] “install a chart” and helm upgrade [RELEASE] [CHART] “upgrade a release”; the displayed/downloaded script therefore tries to create an already-named release and fails instead of reproducing the UI action.
Useful? React with 👍 / 👎.
| s3ProfileName: s3ProfileName_pinned, | ||
| helmValuesPatch | ||
| helmValuesPatch, | ||
| helmReleaseName: helmReleaseName_pinned |
There was a problem hiding this comment.
When a multi-project user opens Modify and then changes project in the header, the existing projectChanged handler re-runs initialization with { ...params }, so this new pin is retained even though the API's ONYXIA-PROJECT header now selects the new project. Pressing Upgrade then targets the old release name in the new namespace, which normally fails and can upgrade an unrelated same-named release. Clear or revalidate the pinned target on project changes, or leave the upgrade flow.
Useful? React with 👍 / 👎.
Here is an example of using the PR for a service upgrade feature (InseeFrLab/onyxia-api#685).
It was coded quickly, just to check that the feature works end to end. It is meant as a proposal: feel free to modify it as you see fit, or to redo it completely.
"0","true").coerceHelmValuesPatchconverts them according to the chart's JSON schema before filling the form.