Fix helm upgrade secrets#16676
Draft
sowmya-sl wants to merge 7 commits into
Draft
Conversation
Use a RegistryClientSetter interface so applyBasicAuthFromUserCredentials works with both action.Install and action.Upgrade via their shared ChartPathOptions. Persist the basic-auth secret name as a chart annotation (helm.openshift.io/auth-secret) during install and propagate it on upgrade so authenticated registries remain accessible across the release lifecycle.
Ensure addAuthSecretAnnotation is called when rewriting chart metadata so subsequent upgrades continue to resolve registry credentials.
Credentials were being set after LocateChart, which is the call that actually contacts the registry. Move applyBasicAuthFromUserCredentials before LocateChart so the registry client has auth headers when pulling.
Surface the persisted auth secret annotation (helm.openshift.io/auth-secret) in the upgrade UI so users can view and change the credentials used for authenticated OCI/HTTP(S) registries. The secret dropdown is pre-populated with the previously used secret and shows a warning if it no longer exists in the namespace. Backend improvements: - Accept basicAuthSecretName in UpgradeReleaseAsync and pass it through the handler chain - Return a descriptive error when registry returns 401 and no auth secret is configured, guiding the user to select a secret - Generalize applyBasicAuthFromUserCredentials to accept ChartPathOptions and RegistryClientSetter interface for reuse across install/upgrade Co-authored-by: Cursor <cursoragent@cursor.com>
Add a Create new Secret action to Helm URL/install-upgrade forms and introduce a dedicated modal to create basic-auth Secrets inline. Wire the selected secret through upgrade/install requests and persist auth-secret metadata so URL-based upgrades can reuse credentials.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/core
Contributor
WalkthroughThis PR adds basic-auth Secret support to Helm install and upgrade flows, including secret creation from the URL form, secret selection in the upgrade form, backend propagation of the secret name, and matching Helm label and test-string updates. ChangesHelm basic-auth secret flow and text updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (2)
pkg/helm/handlers/handler_test.go (1)
138-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the forwarded basic-auth Secret name in the handler fake.
The fake accepts
basicAuthSecretNamebut ignores it, so a regression inHandleUpgradeReleaseAsyncpassingreq.BasicAuthSecretNamewould not fail tests. Consider extending the helper with an expected value and checking it in the closure. As per coding guidelines, “Make sure the tests pass, and add any new tests as appropriate.”🤖 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 `@pkg/helm/handlers/handler_test.go` around lines 138 - 139, The handler test fake for fakeUpgradeReleaseAsync currently accepts basicAuthSecretName but never verifies it, so it can miss regressions in HandleUpgradeReleaseAsync forwarding req.BasicAuthSecretName. Update the helper to take an expected basicAuthSecretName value and assert inside the returned closure that the received argument matches it, using the existing fakeUpgradeReleaseAsync symbol so the test fails if the handler stops passing the secret name correctly.Source: Coding guidelines
pkg/helm/actions/upgrade_release_test.go (1)
509-509: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for non-empty and persisted auth Secret upgrades.
These call sites only pass
"", so the new selected Secret path, annotation fallback path, and missing/malformed Secret failure behavior are untested. A table case withhelm.openshift.io/auth-secretplus a matching basic-auth Secret would catch regressions in this PR’s core upgrade flow. As per coding guidelines, “Make sure the tests pass, and add any new tests as appropriate.”Also applies to: 605-605, 697-697
🤖 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 `@pkg/helm/actions/upgrade_release_test.go` at line 509, Add test coverage in UpgradeReleaseAsync call sites to exercise the persisted auth Secret flow instead of only passing an empty secret name. Update the relevant table-driven cases in upgrade_release_test.go to cover helm.openshift.io/auth-secret, the annotation fallback, and failure handling for missing or malformed basic-auth Secrets so the selected Secret path and upgrade behavior are validated. Use UpgradeReleaseAsync and the related auth Secret resolution logic as the main symbols to locate the affected tests and add the new cases.Source: Coding guidelines
🤖 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
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx`:
- Around line 118-128: Update HelmInstallUpgradeForm so upgrade flows are not
gated by an existing basicAuthSecretName; in the form logic around
showAuthSecret, secretMissing, and the selector rendering, allow users to open
the auth secret selector even when the persisted helm.openshift.io/auth-secret
annotation is absent. Then make the upgrade path let them choose or create a
Secret and persist that selection back into the upgrade state, using the
existing HelmActionType.Upgrade and useSecretResources(namespace) flow.
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx`:
- Around line 73-76: The Helm basic-auth secret modal can currently create
Secrets that the backend will reject because it omits username when empty, while
GetUserCredentials expects both username and password. Update
HelmCreateBasicAuthSecretModal so the generated Secret always includes a
username field (and keep the form validation aligned with that requirement),
using the existing username/password handling in the modal’s secret payload to
prevent creating unusable auth Secrets.
- Around line 45-54: The modal close path in HelmCreateBasicAuthSecretModal’s
closeModal is blocked by the inProgress guard, so a successful create from
handleCreate cannot dismiss the dialog. Update the create flow so the modal can
close after a success, either by clearing inProgress before calling closeModal
or by allowing closeModal to bypass the guard for successful completion, and
make sure the same behavior is applied to the related create/submit path
referenced in handleCreate.
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 163-173: The selected auth Secret is only applied inside the
rel.Chart.Metadata.Annotations nil-check, so releases without annotations skip
basicAuthSecretName and may fetch the chart unauthenticated. Move the
basicAuthSecretName handling in upgrade_release.go outside the
rel.Chart.Metadata.Annotations block in the upgrade flow, keeping the chart_url
annotation lookup conditional but always honoring the user-selected Secret
before falling back to helmAuthSecretAnnotation.
- Around line 210-219: Credential setup errors in the upgrade flow are only
logged and then execution continues, which can lead to an unauthenticated chart
lookup and stale auth-secret state. In `UpgradeRelease`, treat failures from
`GetUserCredentials` and `applyBasicAuthFromUserCredentials` as fatal by
returning the error immediately instead of falling through to `LocateChart`.
Keep the check around `auth_secret` in this block, and make sure any selected
Secret that is missing, malformed, or cannot be used by the registry client
stops the upgrade path before chart resolution.
---
Nitpick comments:
In `@pkg/helm/actions/upgrade_release_test.go`:
- Line 509: Add test coverage in UpgradeReleaseAsync call sites to exercise the
persisted auth Secret flow instead of only passing an empty secret name. Update
the relevant table-driven cases in upgrade_release_test.go to cover
helm.openshift.io/auth-secret, the annotation fallback, and failure handling for
missing or malformed basic-auth Secrets so the selected Secret path and upgrade
behavior are validated. Use UpgradeReleaseAsync and the related auth Secret
resolution logic as the main symbols to locate the affected tests and add the
new cases.
In `@pkg/helm/handlers/handler_test.go`:
- Around line 138-139: The handler test fake for fakeUpgradeReleaseAsync
currently accepts basicAuthSecretName but never verifies it, so it can miss
regressions in HandleUpgradeReleaseAsync forwarding req.BasicAuthSecretName.
Update the helper to take an expected basicAuthSecretName value and assert
inside the returned closure that the received argument matches it, using the
existing fakeUpgradeReleaseAsync symbol so the test fails if the handler stops
passing the secret name correctly.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b166576-6636-476e-abca-97e4a270547d
📒 Files selected for processing (12)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsxfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsxfrontend/public/locales/en/public.jsonpkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/upgrade_release.gopkg/helm/actions/upgrade_release_test.gopkg/helm/handlers/handler_test.gopkg/helm/handlers/handlers.go
Comment on lines
+118
to
+128
| const showAuthSecret = helmAction === HelmActionType.Upgrade && !!basicAuthSecretName; | ||
| const secretResources = useSecretResources(namespace); | ||
| const autocompleteFilter = (strText: string, item: any): boolean => | ||
| fuzzy(strText, item?.props?.name); | ||
| const secretMissing = useMemo(() => { | ||
| if (!showAuthSecret || !secretResources[0]?.loaded) { | ||
| return false; | ||
| } | ||
| const secrets = secretResources[0]?.data ?? []; | ||
| return !secrets.some((s) => s?.metadata?.name === basicAuthSecretName); | ||
| }, [showAuthSecret, secretResources, basicAuthSecretName]); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Upgrades without a persisted auth annotation cannot add one.
Because showAuthSecret requires an existing basicAuthSecretName, the selector is hidden for upgrades that don't already have helm.openshift.io/auth-secret. That prevents users from fixing an unauthorized upgrade by selecting or creating a Secret in the first place.
Suggested fix
- const showAuthSecret = helmAction === HelmActionType.Upgrade && !!basicAuthSecretName;
+ const showAuthSecret = helmAction === HelmActionType.Upgrade;Also applies to: 218-252
🤖 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
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx`
around lines 118 - 128, Update HelmInstallUpgradeForm so upgrade flows are not
gated by an existing basicAuthSecretName; in the form logic around
showAuthSecret, secretMissing, and the selector rendering, allow users to open
the auth secret selector even when the persisted helm.openshift.io/auth-secret
annotation is absent. Then make the upgrade path let them choose or create a
Secret and persist that selection back into the upgrade state, using the
existing HelmActionType.Upgrade and useSecretResources(namespace) flow.
Comment on lines
+45
to
+54
| const closeModal = () => { | ||
| if (inProgress) { | ||
| return; | ||
| } | ||
| if (document.activeElement instanceof HTMLElement) { | ||
| document.activeElement.blur(); | ||
| } | ||
| onClose?.(); | ||
| closeOverlay(); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Successful creates never close the modal.
handleCreate() calls closeModal() while inProgress is still true, so the guard on Line 46 returns early and the overlay stays open after a successful create.
Suggested fix
- const closeModal = () => {
- if (inProgress) {
- return;
- }
+ const closeModal = (force = false) => {
+ if (inProgress && !force) {
+ return;
+ }
if (document.activeElement instanceof HTMLElement) {
document.activeElement.blur();
}
onClose?.();
closeOverlay();
@@
- closeModal();
+ closeModal(true);
// Keep form update separate so a parent callback failure cannot block modal close.
save?.(createdSecretName);Also applies to: 78-81
🤖 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
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx`
around lines 45 - 54, The modal close path in HelmCreateBasicAuthSecretModal’s
closeModal is blocked by the inProgress guard, so a successful create from
handleCreate cannot dismiss the dialog. Update the create flow so the modal can
close after a success, either by clearing inProgress before calling closeModal
or by allowing closeModal to bypass the guard for successful completion, and
make sure the same behavior is applied to the related create/submit path
referenced in handleCreate.
Comment on lines
+73
to
+76
| stringData: { | ||
| ...(username ? { username } : {}), | ||
| password, | ||
| }, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The modal can create Secrets the backend cannot use.
The UI treats username as optional and omits that key entirely, but pkg/helm/actions/install_chart.go's GetUserCredentials rejects Secrets missing either username or password. A Secret created here without a username will fail the install/upgrade auth flow later. Based on learnings, English-only i18n additions are fine in this repo and do not need separate l10n updates.
Suggested fix
- const isCreateDisabled = !secretName.trim() || !password;
+ const isCreateDisabled = !secretName.trim() || !username.trim() || !password;
@@
- <FormGroup label={t('public~Secret username')} fieldId="helm-secret-username">
+ <FormGroup
+ label={t('public~Secret username')}
+ isRequired
+ fieldId="helm-secret-username"
+ >
<TextInput
@@
- <HelperTextItem>
- {t('helm-plugin~Optional username for OCI/HTTP(S) authentication.')}
- </HelperTextItem>
+ <HelperTextItem>{t('helm-plugin~Username for OCI/HTTP(S) authentication.')}</HelperTextItem>Also applies to: 122-137
🤖 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
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx`
around lines 73 - 76, The Helm basic-auth secret modal can currently create
Secrets that the backend will reject because it omits username when empty, while
GetUserCredentials expects both username and password. Update
HelmCreateBasicAuthSecretModal so the generated Secret always includes a
username field (and keep the form validation aligned with that requirement),
using the existing username/password handling in the modal’s secret payload to
prevent creating unusable auth Secrets.
Source: Learnings
Comment on lines
+163
to
+173
| auth_secret := "" | ||
| // Before proceeding, check if chart URL is present as an annotation | ||
| if rel.Chart.Metadata.Annotations != nil { | ||
| if chart_url, ok := rel.Chart.Metadata.Annotations["chart_url"]; chartUrl == "" && ok { | ||
| chartUrl = chart_url | ||
| } | ||
| if basicAuthSecretName != "" { | ||
| auth_secret = basicAuthSecretName | ||
| } else if authSecret, ok := rel.Chart.Metadata.Annotations[helmAuthSecretAnnotation]; ok { | ||
| auth_secret = authSecret | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the selected Secret even when release annotations are absent.
Line 169 only assigns basicAuthSecretName inside the rel.Chart.Metadata.Annotations != nil block, so releases without existing annotations drop the user-selected Secret and locate the chart unauthenticated.
Proposed fix
- auth_secret := ""
+ auth_secret := basicAuthSecretName
// Before proceeding, check if chart URL is present as an annotation
- if rel.Chart.Metadata.Annotations != nil {
+ if rel.Chart.Metadata != nil && rel.Chart.Metadata.Annotations != nil {
if chart_url, ok := rel.Chart.Metadata.Annotations["chart_url"]; chartUrl == "" && ok {
chartUrl = chart_url
}
- if basicAuthSecretName != "" {
- auth_secret = basicAuthSecretName
- } else if authSecret, ok := rel.Chart.Metadata.Annotations[helmAuthSecretAnnotation]; ok {
- auth_secret = authSecret
+ if auth_secret == "" {
+ if authSecret, ok := rel.Chart.Metadata.Annotations[helmAuthSecretAnnotation]; ok {
+ auth_secret = authSecret
+ }
}
}📝 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
| auth_secret := "" | |
| // Before proceeding, check if chart URL is present as an annotation | |
| if rel.Chart.Metadata.Annotations != nil { | |
| if chart_url, ok := rel.Chart.Metadata.Annotations["chart_url"]; chartUrl == "" && ok { | |
| chartUrl = chart_url | |
| } | |
| if basicAuthSecretName != "" { | |
| auth_secret = basicAuthSecretName | |
| } else if authSecret, ok := rel.Chart.Metadata.Annotations[helmAuthSecretAnnotation]; ok { | |
| auth_secret = authSecret | |
| } | |
| auth_secret := basicAuthSecretName | |
| // Before proceeding, check if chart URL is present as an annotation | |
| if rel.Chart.Metadata != nil && rel.Chart.Metadata.Annotations != nil { | |
| if chart_url, ok := rel.Chart.Metadata.Annotations["chart_url"]; chartUrl == "" && ok { | |
| chartUrl = chart_url | |
| } | |
| if auth_secret == "" { | |
| if authSecret, ok := rel.Chart.Metadata.Annotations[helmAuthSecretAnnotation]; ok { | |
| auth_secret = authSecret | |
| } | |
| } | |
| } |
🤖 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 `@pkg/helm/actions/upgrade_release.go` around lines 163 - 173, The selected
auth Secret is only applied inside the rel.Chart.Metadata.Annotations nil-check,
so releases without annotations skip basicAuthSecretName and may fetch the chart
unauthenticated. Move the basicAuthSecretName handling in upgrade_release.go
outside the rel.Chart.Metadata.Annotations block in the upgrade flow, keeping
the chart_url annotation lookup conditional but always honoring the
user-selected Secret before falling back to helmAuthSecretAnnotation.
Comment on lines
+210
to
+219
| if auth_secret != "" { | ||
| klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) | ||
| userCredentials, err := GetUserCredentials(coreClient, releaseNamespace, auth_secret) | ||
| if err != nil { | ||
| klog.Errorf("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err) | ||
| } else { | ||
| if err := applyBasicAuthFromUserCredentials(&client.ChartPathOptions, client, userCredentials); err != nil { | ||
| klog.Errorf("Failed to apply auth from secret %s for release %s/%s: %v", auth_secret, releaseNamespace, releaseName, err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return credential setup failures instead of continuing unauthenticated.
If the persisted or selected Secret is missing, malformed, or registry-client setup fails, this logs the error but still calls LocateChart; that can produce a misleading 401 or even persist an auth-secret annotation that was never applied.
Proposed fix
if auth_secret != "" {
klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName)
userCredentials, err := GetUserCredentials(coreClient, releaseNamespace, auth_secret)
if err != nil {
- klog.Errorf("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err)
+ return nil, fmt.Errorf("failed to get user credentials for release upgrade %s/%s: %w", releaseNamespace, releaseName, err)
+ }
+ if err := applyBasicAuthFromUserCredentials(&client.ChartPathOptions, client, userCredentials); err != nil {
+ return nil, fmt.Errorf("failed to apply auth from secret %s for release %s/%s: %w", auth_secret, releaseNamespace, releaseName, err)
- } else {
- if err := applyBasicAuthFromUserCredentials(&client.ChartPathOptions, client, userCredentials); err != nil {
- klog.Errorf("Failed to apply auth from secret %s for release %s/%s: %v", auth_secret, releaseNamespace, releaseName, err)
- }
}
}📝 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 auth_secret != "" { | |
| klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) | |
| userCredentials, err := GetUserCredentials(coreClient, releaseNamespace, auth_secret) | |
| if err != nil { | |
| klog.Errorf("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err) | |
| } else { | |
| if err := applyBasicAuthFromUserCredentials(&client.ChartPathOptions, client, userCredentials); err != nil { | |
| klog.Errorf("Failed to apply auth from secret %s for release %s/%s: %v", auth_secret, releaseNamespace, releaseName, err) | |
| } | |
| } | |
| if auth_secret != "" { | |
| klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) | |
| userCredentials, err := GetUserCredentials(coreClient, releaseNamespace, auth_secret) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get user credentials for release upgrade %s/%s: %w", releaseNamespace, releaseName, err) | |
| } | |
| if err := applyBasicAuthFromUserCredentials(&client.ChartPathOptions, client, userCredentials); err != nil { | |
| return nil, fmt.Errorf("failed to apply auth from secret %s for release %s/%s: %w", auth_secret, releaseNamespace, releaseName, err) | |
| } | |
| } |
🤖 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 `@pkg/helm/actions/upgrade_release.go` around lines 210 - 219, Credential setup
errors in the upgrade flow are only logged and then execution continues, which
can lead to an unauthenticated chart lookup and stale auth-secret state. In
`UpgradeRelease`, treat failures from `GetUserCredentials` and
`applyBasicAuthFromUserCredentials` as fatal by returning the error immediately
instead of falling through to `LocateChart`. Keep the check around `auth_secret`
in this block, and make sure any selected Secret that is missing, malformed, or
cannot be used by the registry client stops the upgrade path before chart
resolution.
Normalize Helm plugin copy to use Helm release and Helm Chart(s) across UI, i18n strings, and integration tests. Update auth secret help text to use "username" and "password" consistently in URL/install-upgrade forms and locales.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@frontend/packages/helm-plugin/locales/fr/helm-plugin.json`:
- Around line 153-154: The French Helm plugin translations have spacing
mismatches in the templated strings, which causes concatenated labels to render
incorrectly. Update the entries for the keys with appVersion and chartRepoName
in helm-plugin.json so they preserve the same leading and internal spacing as
the English source; specifically, keep the leading space before the App Version
fragment and add the missing space before the chartRepoName placeholder in the
translation values.
In `@frontend/packages/helm-plugin/locales/ja/helm-plugin.json`:
- Line 148: The Japanese locale entry for Repositories is still untranslated and
should be replaced with an appropriate Japanese string to match the other
locales. Update the locale value in the helm-plugin JSON for the Repositories
key, using the existing translation pattern from the ko and zh files as a guide
while keeping the same key and JSON structure.
In `@frontend/packages/helm-plugin/locales/ko/helm-plugin.json`:
- Line 126: The Korean translation for the Helm chart URL example is malformed
and should match the source text. Update the value in helm-plugin.json so the
OCI example uses the correct registry path from the original string, and remove
the duplicated mycharts/mychart segment while keeping the rest of the
translation unchanged.
In `@frontend/packages/helm-plugin/locales/zh/helm-plugin.json`:
- Line 158: Update the Chinese translation for the "Upgrade Helm release" entry
in helm-plugin.json so it uses the correct upgrade wording instead of creation
wording. Locate the localized string keyed by "Upgrade Helm release" in the zh
locale file and change the value from 创建 Helm 发行 to the appropriate 升级
equivalent to match the action label used by the Helm plugin UI.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f27dcfe-24ac-4794-883c-a6837b9afc8e
📒 Files selected for processing (50)
frontend/packages/helm-plugin/console-extensions.jsonfrontend/packages/helm-plugin/integration-tests/features/helm-release.featurefrontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release-after-upgrade.featurefrontend/packages/helm-plugin/integration-tests/features/helm/actions-on-helm-release.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-feature-flag.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-navigation.featurefrontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.featurefrontend/packages/helm-plugin/integration-tests/features/helm/install-helm-chart.featurefrontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.featurefrontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.featurefrontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-details-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.tsfrontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-navigation.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-release.tsfrontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.tsfrontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/locales/es/helm-plugin.jsonfrontend/packages/helm-plugin/locales/fr/helm-plugin.jsonfrontend/packages/helm-plugin/locales/ja/helm-plugin.jsonfrontend/packages/helm-plugin/locales/ko/helm-plugin.jsonfrontend/packages/helm-plugin/locales/zh/helm-plugin.jsonfrontend/packages/helm-plugin/src/actions/add-resources.tsxfrontend/packages/helm-plugin/src/actions/creators.tsfrontend/packages/helm-plugin/src/catalog/utils/catalog-utils.tsxfrontend/packages/helm-plugin/src/components/__tests__/helm-release-mock-data.tsfrontend/packages/helm-plugin/src/components/details-page/HelmReleaseDetails.tsxfrontend/packages/helm-plugin/src/components/details-page/history/HelmReleaseHistory.tsxfrontend/packages/helm-plugin/src/components/details-page/overview/HelmReleaseOverview.tsxfrontend/packages/helm-plugin/src/components/details-page/overview/__tests__/HelmReleaseOverview.spec.tsxfrontend/packages/helm-plugin/src/components/forms/__tests__/HelmInstallUpgradeForm.spec.tsxfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLInstallForm.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmReleaseListPage.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmReleaseListRow.tsxfrontend/packages/helm-plugin/src/components/list-page/HelmTabbedPage.tsxfrontend/packages/helm-plugin/src/models/helm.tsfrontend/packages/helm-plugin/src/topology/helmFilters.tsfrontend/packages/helm-plugin/src/utils/__tests__/helm-utils.spec.tsfrontend/packages/helm-plugin/src/utils/helm-utils.tsfrontend/public/locales/en/public.json
✅ Files skipped from review due to trivial changes (27)
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/upgrade-helm-release-page.ts
- frontend/packages/helm-plugin/integration-tests/support/constants/static-text/helm-text.ts
- frontend/packages/helm-plugin/src/components/details-page/overview/tests/HelmReleaseOverview.spec.tsx
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm-compatibility.ts
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-compatibility.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-page-tabs.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/install-url-chart.feature
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-feature-flag.feature
- frontend/packages/helm-plugin/src/utils/tests/helm-utils.spec.ts
- frontend/packages/helm-plugin/integration-tests/support/pages/helm/helm-page.ts
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.ts
- frontend/packages/helm-plugin/src/models/helm.ts
- frontend/packages/helm-plugin/src/components/list-page/HelmReleaseListPage.tsx
- frontend/packages/helm-plugin/src/components/forms/tests/HelmInstallUpgradeForm.spec.tsx
- frontend/packages/helm-plugin/src/topology/helmFilters.ts
- frontend/packages/helm-plugin/src/catalog/utils/catalog-utils.tsx
- frontend/packages/helm-plugin/src/components/tests/helm-release-mock-data.ts
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx
- frontend/packages/helm-plugin/integration-tests/features/helm/install-helm-chart.feature
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLInstallForm.tsx
- frontend/packages/helm-plugin/console-extensions.json
- frontend/packages/helm-plugin/src/components/list-page/HelmReleaseList.tsx
- frontend/packages/helm-plugin/src/components/details-page/history/HelmReleaseHistory.tsx
- frontend/packages/helm-plugin/integration-tests/features/helm/topology-helm-release.feature
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/helm/helm.ts
- frontend/packages/helm-plugin/src/components/list-page/HelmTabbedPage.tsx
- frontend/packages/helm-plugin/integration-tests/features/helm/helm-installation-view.feature
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/public/locales/en/public.json
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx
- frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx
Comment on lines
153
to
154
| " / App Version {{appVersion}}": "/Version de l’application {{appVersion}}", | ||
| " (Provided by {{chartRepoName}})": " (Fourni par{{chartRepoName}})", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing/incorrect spacing in templated French strings.
- Line 154:
" (Fourni par{{chartRepoName}})"is missing the space before the placeholder, rendering aspar<name>. The English source preserves it:" (Provided by {{chartRepoName}})". - Line 153:
"/Version de l’application {{appVersion}}"drops the leading/surrounding spaces present in the English key" / App Version {{appVersion}}". Since these fragments are concatenated onto preceding text, the missing leading space can run into the adjacent label.
🌐 Proposed fix
- " / App Version {{appVersion}}": "/Version de l’application {{appVersion}}",
+ " / App Version {{appVersion}}": " / Version de l’application {{appVersion}}",
" (Provided by {{chartRepoName}})": " (Fourni par{{chartRepoName}})",- " (Provided by {{chartRepoName}})": " (Fourni par{{chartRepoName}})",
+ " (Provided by {{chartRepoName}})": " (Fourni par {{chartRepoName}})",📝 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
| " / App Version {{appVersion}}": "/Version de l’application {{appVersion}}", | |
| " (Provided by {{chartRepoName}})": " (Fourni par{{chartRepoName}})", | |
| " / App Version {{appVersion}}": " / Version de l’application {{appVersion}}", | |
| " (Provided by {{chartRepoName}})": " (Fourni par {{chartRepoName}})", |
🤖 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 `@frontend/packages/helm-plugin/locales/fr/helm-plugin.json` around lines 153 -
154, The French Helm plugin translations have spacing mismatches in the
templated strings, which causes concatenated labels to render incorrectly.
Update the entries for the keys with appVersion and chartRepoName in
helm-plugin.json so they preserve the same leading and internal spacing as the
English source; specifically, keep the leading space before the App Version
fragment and add the missing space before the chartRepoName placeholder in the
translation values.
| "No Helm releases found": "Helm リリースが見つかりません", | ||
| "Browse the catalog to discover available Helm Chart(s)": "カタログを参照して利用可能な Helm チャートを検出", | ||
| "Helm Chart URL": "Helm チャートの URL", | ||
| "Repositories": "Repositories", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Untranslated string.
"Repositories": "Repositories" is left in English while the ko (리포지토리) and zh (存储库) locales translate it. Confirm whether a Japanese translation should be supplied here.
🤖 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 `@frontend/packages/helm-plugin/locales/ja/helm-plugin.json` at line 148, The
Japanese locale entry for Repositories is still untranslated and should be
replaced with an appropriate Japanese string to match the other locales. Update
the locale value in the helm-plugin JSON for the Repositories key, using the
existing translation pattern from the ko and zh files as a guide while keeping
the same key and JSON structure.
| "To install a Helm Chart, enter the chart URL - Open Container Initiative (OCI) URL or HTTP/HTTPS tar file and version.": "Helm 차트를 설치하려면 차트 URL - OCI(Open Container Initiative) URL 또는 HTTP/HTTPS tar 파일 및 버전을 입력합니다.", | ||
| "Chart URL": "차트 URL", | ||
| "The OCI URL or HTTP/HTTPS tar file for the Helm chart; for example - oci://registry.example.com/charts/mychart or https://example.com/chart-1.0.0.tgz.": "Helm 차트의 OCI URL 또는 HTTP/HTTPS tar 파일(예: - oci://registry.example.com/charts/mycharts/mychart) 또는 https://example.com/chart-1.0.0.tgz", | ||
| "The OCI URL or HTTP/HTTPS tar file for the Helm Chart; for example - oci://registry.example.com/charts/mychart or https://example.com/chart-1.0.0.tgz.": "Helm 차트의 OCI URL 또는 HTTP/HTTPS tar 파일(예: - oci://registry.example.com/charts/mycharts/mychart) 또는 https://example.com/chart-1.0.0.tgz", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect ko value vs en for this key to confirm the example URL discrepancy
rg -n 'OCI URL or HTTP/HTTPS tar file for the Helm Chart' \
frontend/packages/helm-plugin/locales/ko/helm-plugin.json \
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonRepository: openshift/console
Length of output: 510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding locale entries and locate the English source key/value.
for f in frontend/packages/helm-plugin/locales/ko/helm-plugin.json \
frontend/packages/helm-plugin/locales/en/helm-plugin.json; do
echo "=== $f ==="
wc -l "$f"
done
echo "=== KO context ==="
sed -n '118,132p' frontend/packages/helm-plugin/locales/ko/helm-plugin.json
echo "=== EN matches ==="
rg -n '"The OCI URL or HTTP/HTTPS tar file for the Helm Chart; for example - oci://registry.example.com/charts/mychart or https://example.com/chart-1.0.0.tgz\."' \
frontend/packages/helm-plugin/locales/en/helm-plugin.json || true
echo "=== EN context around similar text ==="
rg -n -C 1 'OCI URL or HTTP/HTTPS tar file for the Helm Chart|oci://registry.example.com/charts/mychart|chart-1.0.0.tgz' \
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonRepository: openshift/console
Length of output: 2833
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding locale entries and locate the English source key/value.
for f in frontend/packages/helm-plugin/locales/ko/helm-plugin.json \
frontend/packages/helm-plugin/locales/en/helm-plugin.json; do
echo "=== $f ==="
wc -l "$f"
done
echo "=== KO context ==="
sed -n '118,132p' frontend/packages/helm-plugin/locales/ko/helm-plugin.json
echo "=== EN matches ==="
rg -n '"The OCI URL or HTTP/HTTPS tar file for the Helm Chart; for example - oci://registry.example.com/charts/mychart or https://example.com/chart-1.0.0.tgz\."' \
frontend/packages/helm-plugin/locales/en/helm-plugin.json || true
echo "=== EN context around similar text ==="
rg -n -C 1 'OCI URL or HTTP/HTTPS tar file for the Helm Chart|oci://registry.example.com/charts/mychart|chart-1.0.0.tgz' \
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonRepository: openshift/console
Length of output: 2833
Fix the malformed Helm chart example in frontend/packages/helm-plugin/locales/ko/helm-plugin.json.
The Korean translation has oci://registry.example.com/charts/mycharts/mychart instead of oci://registry.example.com/charts/mychart; remove the duplicated path segment and keep the example consistent with the source string.
🤖 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 `@frontend/packages/helm-plugin/locales/ko/helm-plugin.json` at line 126, The
Korean translation for the Helm chart URL example is malformed and should match
the source text. Update the value in helm-plugin.json so the OCI example uses
the correct registry path from the original string, and remove the duplicated
mycharts/mychart segment while keeping the rest of the translation unchanged.
| "Create Helm release": "创建 Helm 发行版本", | ||
| "The Helm release can be created by completing the form. Default values may be provided by the Helm Chart authors.": "Helm 发行版本可通过填写表单进行创建。Helm Chart 的作者可能会提供默认值。", | ||
| "The Helm release can be created by manually entering YAML or JSON definitions.": "Helm 发行版本可通过手动输入 YAML 或 JSON 定义进行创建。", | ||
| "Upgrade Helm release": "创建 Helm 发行", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Incorrect Chinese translation for "Upgrade Helm release".
The value "创建 Helm 发行" means "Create Helm release", not "Upgrade". This would mislabel the upgrade action/heading. 创建 should be 升级.
🌐 Proposed fix
- "Upgrade Helm release": "创建 Helm 发行",
+ "Upgrade Helm release": "升级 Helm 发行",📝 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
| "Upgrade Helm release": "创建 Helm 发行", | |
| "Upgrade Helm release": "升级 Helm 发行", |
🤖 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 `@frontend/packages/helm-plugin/locales/zh/helm-plugin.json` at line 158,
Update the Chinese translation for the "Upgrade Helm release" entry in
helm-plugin.json so it uses the correct upgrade wording instead of creation
wording. Locate the localized string keyed by "Upgrade Helm release" in the zh
locale file and change the value from 创建 Helm 发行 to the appropriate 升级
equivalent to match the action label used by the Helm plugin UI.
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
1 participant
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.
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit