HELM-763: Fix helm upgrade secrets#16676
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
needs-rebase
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 | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| const closeModal = () => { | ||
| if (inProgress) { | ||
| return; | ||
| } | ||
| if (document.activeElement instanceof HTMLElement) { | ||
| document.activeElement.blur(); | ||
| } | ||
| onClose?.(); | ||
| closeOverlay(); | ||
| }; |
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.
| stringData: { | ||
| ...(username ? { username } : {}), | ||
| password, | ||
| }, |
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
| 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 | ||
| } |
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.
| 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.
| 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) | ||
| } | ||
| } |
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.
| 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.
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
7468da3 to
243b4d6
Compare
openshift-ci
Bot
added
component/sdk
05376cf to
955ad3f
Compare
|
/test all |
1 similar comment
|
/test all |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cd3ddc2 to
221ca0b
Compare
openshift-ci-robot
added
the
jira/valid-reference
|
@sowmya-sl: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
verified
|
@sowmya-sl: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
All of the frontend/packages/helm-plugin/integration-tests/features/helm/ files, as well as many of the other files in this PR, are being deleted in #16711.
Before I review this PR, we need to determine how we're going to resolve this conflict. (E.g., perhaps you should base your branch on Vikram's, but I don't think he's got all the tests converted...so I'm not sure what to suggest...perhaps coordinate with him?)
|
/retest |
1 similar comment
|
/retest |
…e microcopy - Add HelmCreateBasicAuthSecretModal for creating basic auth secrets during OCI/HTTP chart installation from URL - Extend HelmInstallUpgradeForm and HelmURLChartForm to support basic auth secret selection for authenticated chart repositories - Add upgrade support with basic auth secret propagation via chart annotations in get_chart.go, install_chart.go, and upgrade_release.go - Update handler to pass basicAuthSecretName through GetChartFromURL - Normalize UI microcopy: lowercase "release" in "Helm release", use "Helm Chart(s)" consistently across console-extensions.json, integration test features, step definitions, and page objects - Update i18n keys in helm-plugin and public locales Co-authored-by: Cursor <cursoragent@cursor.com>
…cret for URL-installed charts Propagate the 'installation: url_install' annotation from the previous release to the upgraded chart in both UpgradeRelease and UpgradeReleaseAsync, so the frontend can distinguish URL-installed charts from catalog-installed ones across upgrades. Use this annotation on the frontend to show the basic auth secret section only for URL-based installs and upgrades, not for catalog charts. Also unexport the unused HelmCreateBasicAuthSecretModalProps interface to fix the CI unused-exports check. Co-authored-by: Cursor <cursoragent@cursor.com>
…lity - Make username required in the basic auth secret modal and fix modal close so Cancel/X always dismiss regardless of in-progress state - Fix upgrade_release.go to prefer explicit basicAuthSecretName over annotation fallback, add nil-safety for chart metadata, and return errors instead of logging-and-continuing on credential failures - Show auth secret dropdown only for URL-installed charts (based on installation annotation), not for all catalog installs - Remove helm-plugin~ prefix from direct t() calls where useTranslation already provides the namespace; retain prefix in nameKey values consumed by NavBar with a different namespace context Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… selection Users previously had no way to deselect a basic auth secret once one was chosen. Adding a "None" action item lets users explicitly clear the secret field without needing to create a new one.
Previously, selecting "None" for the auth secret sent an empty string, which was indistinguishable from "no selection." The backend would then re-inherit the auth secret from the previous release's annotations, making it impossible to clear. Use a "__none__" sentinel value so the backend can differentiate between "not set" and "explicitly cleared," and only send basic_auth_secret_name for URL-based installs.
d3e265b to
11dbdab
Compare
|
@sowmya-sl: This pull request references HELM-763 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
| "href": "/catalog/ns/:namespace?catalogType=HelmChart", | ||
| "label": "%helm-plugin~Helm Chart%", | ||
| "description": "%helm-plugin~Browse the catalog to discover and install Helm Charts%", | ||
| "description": "%helm-plugin~Browse the catalog to discover and install Helm Chart(s)%", |
There was a problem hiding this comment.
Nit: Is there a reason for the (s) in Helm Chart(s)? This pattern isn't used by any other catalog type in the console. Was this an intentional style decision, or could we revert to the previous Helm Charts?
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
Reviewed for wording, clarity, consistency, voice. Recurring issues: passive voice in a few help-text strings, suggest active voice throughout; capitalization inconsistency for "Secret" and "Chart" across files; dropped article in secret authentication helper text; "public~" namespace prefix leaking (?) into rendered UI strings.
| "Helm Chart repositories": "Helm Chart repositories", | ||
| "Helm Chart": "Helm Chart", | ||
| "Browse the catalog to discover and install Helm Charts": "Browse the catalog to discover and install Helm Charts", | ||
| "Browse the catalog to discover and install Helm Chart(s)": "Browse the catalog to discover and install Helm Chart(s)", |
There was a problem hiding this comment.
Line 8 (+ 10, 11):
-
"Chart(s)" = parenthetical plurals typically avoided in UI text. they're awkward to read; can complicate translation. if it could be more than 1, even if it's not definitively 1, just use "Charts".
-
GLOBAL: note that Helm documentation inconsistently capitalizes "Chart," but more often, it's lowercase. (i'm referring specifically to the phrase "Helm C/chart," not the word "chart" on its own, which they always lowercase.) we should make sure we use just one and i recommend lowercase "chart."
-
flagging that "Chart(s)" isn't used consistently. other strings further down (e.g., "Install Helm Chart from URL") keep singular form. we should align on one pattern throughout.
There was a problem hiding this comment.
- Please check comment .
- HelmChartRepository and ProjectHelmChartRepository objects have Chart capitalized, so I have added Chart with C capital. Thoughts?
- I am okay with keeping it singular. But we need to mention plural at some point right? "Browse the catalog to discover available Helm Charts" ?
| "Select": "Select", | ||
| "Must be a valid OCI URL or a valid HTTP/HTTPS tar file; for example - oci://registry.example.com/chart, https://example.com/chart-1.0.0.tgz.": "Must be a valid OCI URL or a valid HTTP/HTTPS tar file; for example - oci://registry.example.com/chart, https://example.com/chart-1.0.0.tgz.", | ||
| "Failed to create Secret.": "Failed to create Secret.", | ||
| "Create authentication Secret": "Create authentication Secret", |
There was a problem hiding this comment.
128: "Failed to create Secret." should "Secret" be lowercase here? it's not a proper noun or named UI element. suggest "Failed to create secret." for consistency w/sentence-case error messages elsewhere in this file.
129: "Create authentication Secret" uses mixed casing. red hat style calls for sentence case --> "Create authentication secret"
There was a problem hiding this comment.
Secret is a Openshift resource. Should I make it secret nevertheless?
| "Secret for basic authentication": "Secret for basic authentication", | ||
| "None": "None", | ||
| "Select a secret": "Select a secret", | ||
| "Create Secret": "Create Secret", |
There was a problem hiding this comment.
Line 118 vs 129: we have both "Create Secret" and "Create authentication Secret" for what looks like the same action. do these refer to the same UI element?
i recommend GLOBAL: "Create secret" and "Create authentication secret"
although, if they are the same action, should it always say "Create authentication secret" or just "Create secret"? either way, it should be the same every time so as not to confuse users.
| "The Helm Release can be created by manually entering YAML or JSON definitions.": "The Helm Release can be created by manually entering YAML or JSON definitions.", | ||
| "Upgrade Helm Release": "Upgrade Helm Release", | ||
| "Create Helm release": "Create Helm release", | ||
| "The Helm release can be created by completing the form. Default values may be provided by the Helm Chart authors.": "The Helm release can be created by completing the form. Default values may be provided by the Helm Chart authors.", |
There was a problem hiding this comment.
167: might = possibility. may = permission. also, use active voice whenever possible.
--> "You can create the Helm release by completing the form. The Helm Chart authors might have provided default values."
period at end everywhere or no?
if same phrasing repeats elsewhere please fix. GLOBAL
| "None": "None", | ||
| "Select a secret": "Select a secret", | ||
| "Create Secret": "Create Secret", | ||
| "Secret with \"username\" and \"password\" keys for OCI/HTTP(S) authentication.": "Secret with \"username\" and \"password\" keys for OCI/HTTP(S) authentication.", |
There was a problem hiding this comment.
119: article "A" was dropped. Other comparable helper text in this file uses a leading article (e.g., "The version of chart to install.," "The Helm Chart is currently unavailable."), so this looks like an inconsistency rather than an intentional style choice. Suggest restoring it:
--> "A secret with "username" and "password" keys for OCI/HTTP(S) authentication."
| "Helm release": "Helm release", | ||
| "Complete the form to create a Helm release. The Helm chart authors might have provided some default values.": "Complete the form to create a Helm release. The Helm chart authors might have provided some default values.", | ||
| "Install Helm Chart from Helm registry.": "Install Helm Chart from Helm registry.", | ||
| "Complete the form to create a Helm release. Default values might already be set by the chart.": "Complete the form to create a Helm release. Default values might already be set by the chart.", |
There was a problem hiding this comment.
--> The chart might already have set default values.
| subTitle: { | ||
| form: t( | ||
| 'helm-plugin~The Helm Release can be created by completing the form. Default values may be provided by the Helm chart authors.', | ||
| 'helm-plugin~The Helm release can be created by completing the form. Default values may be provided by the Helm Chart authors.', |
There was a problem hiding this comment.
--> You can create a Helm release by completing the form. The Helm Chart authors might have provided default values.
| ), | ||
| yaml: t( | ||
| 'helm-plugin~The Helm Release can be created by manually entering YAML or JSON definitions.', | ||
| 'helm-plugin~The Helm release can be created by manually entering YAML or JSON definitions.', |
There was a problem hiding this comment.
--> You can create a Helm release by manually entering YAML or JSON definitions.
| "Helm Release": "Helm Release", | ||
| "Helm release": "Helm release", | ||
| "For more information on the chart, refer to this <2>README</2>": "For more information on the chart, refer to this <2>README</2>", | ||
| "Helm Chart cannot be installed": "Helm Chart cannot be installed", |
There was a problem hiding this comment.
if we know who/what cannot install the chart, please change to:
The _______ cannot install the Helm Chart.
or
You cannot install the Helm Chart.
| "Upgrade Helm Release": "Upgrade Helm Release", | ||
| "Create Helm release": "Create Helm release", | ||
| "The Helm release can be created by completing the form. Default values may be provided by the Helm Chart authors.": "The Helm release can be created by completing the form. Default values may be provided by the Helm Chart authors.", | ||
| "The Helm release can be created by manually entering YAML or JSON definitions.": "The Helm release can be created by manually entering YAML or JSON definitions.", |
There was a problem hiding this comment.
active voice --> You can create the Helm release by...
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
@sowmya-sl, this is a difficult PR to review. All other things being equal (i.e., Jocelyn's feedback notwithstanding), I would have suggested separate PRs for each of the following:
- replacing
chartwithChartorChart(s) - replacing
Releasewithrelease - everything else....
The first two would be easy to review (or could even be approved without review) because of the nature of the change. The problem is, my eyes are glazing over trying to ignore the first two changes as I look for the third set.
So, depending on how things fall out with the incorporation of Jocelyn's requests (particularly the global ones), it would be good if you could break this PR up into a sequence of PRs. Hopefully, the result would be a few PRs which have lots of files with the same "superficial" change in each, followed by one PR which actually contains the fix that you originally set out to make.
[I made it 5/8's of the way through the files (I did 33 of them), but I'm out of juice...I'll try to finish tomorrow.]
| @@ -1,3 +1,3 @@ | |||
| export const messages = { | |||
| noHelmReleasesFound: 'No Helm Releases found', | |||
| noHelmReleasesFound: 'No Helm releases found', | |||
There was a problem hiding this comment.
I don't think this definition is ever used (should it have been used here?). (I found parameters with the same name, but no obvious references to this definition.)
If this is not being used, we should remove it; if we are using it, perhaps it should refer to the translated string? E.g.,
| noHelmReleasesFound: 'No Helm releases found', | |
| noHelmReleasesFound: t('No Helm releases found'), |
There was a problem hiding this comment.
No — this is correct without t(). This file is part of the Cypress integration test infrastructure, not the application code. It's a constant used in test assertions to verify that the UI displays the expected text:
There was a problem hiding this comment.
used in test assertions
I couldn't find any uses.
| import { useMemo, useState } from 'react'; | ||
| import { TextInputTypes, Grid, GridItem, Button, Alert } from '@patternfly/react-core'; | ||
| import type { FormikProps } from 'formik'; | ||
| import * as fuzzy from 'fuzzysearch'; |
There was a problem hiding this comment.
This statement looks funny...and, I think line 119 is working basically by coincidence. That is, I think things are working they way you want them to only because fuzzysearch happens to export only a single symbol -- if it exported more than one (e.g., in the future), then fuzzy would end up being an object instead of a function, and the reference to fuzzy(...) wouldn't work.
I think you want this to be
| import * as fuzzy from 'fuzzysearch'; | |
| import { fuzzysearch } as fuzzy from 'fuzzysearch'; |
(but, I am definitely not a Typescript/Javascript expert...).
There was a problem hiding this comment.
I see the same idiom used in existing code in frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx (and I think I found two other places), so I guess it's fine...but, if it's not, then you should change it. 😇
| ) { | ||
| return false; | ||
| } | ||
| const secrets = secretResources[0]?.data ?? []; |
There was a problem hiding this comment.
You don't need the ? after secretResources[0] because of the guard at line 125.
There was a problem hiding this comment.
Purely defensive, but yes we don't need.
| <ModalHeader title={t('Create authentication Secret')} /> | ||
| <ModalBody> | ||
| <Form onSubmit={onSubmit}> | ||
| <FormGroup label={t('public~Secret name')} isRequired fieldId="helm-secret-name"> |
There was a problem hiding this comment.
The prefix refers to the public/locales/en/public.json file (or, at least, to the public localization). Secret name is a key to a translation value in the public table; the rendering is determined by the table entry, which is elsewhere.
- Remove public~ i18n namespace usage; use helm-plugin namespace
- Unify "Create Secret" / "Create authentication Secret" to
"Create authentication secret"
- Use active voice: "You can create a Helm release by..."
- Fix might/may usage (might = possibility, may = permission)
- Standardize "Helm Chart(s)" to "Helm Charts" globally
- Standardize ".tar file" and "for example," formatting
- Add missing article in helper text ("A secret with...")
- Unify "Secret for Basic authentication" label across forms
- Lowercase standalone "chart" when not preceded by "Helm"
- Remove unnecessary optional chaining in HelmInstallUpgradeForm
- Update integration tests to reflect UI text changes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
| } | ||
| chartLocation, err := cmd.ChartPathOptions.LocateChart(url, settings) | ||
| if err != nil { | ||
| if basicAuthSecretName == "" && (strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "unauthorized")) { |
There was a problem hiding this comment.
Should the comparison with "unauthorized" be case-blind?
| if err != nil { | ||
| if basicAuthSecretName == "" && (strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "unauthorized")) { | ||
| return nil, fmt.Errorf("error getting chart from URL: %w; registry requires authentication - select a Secret with \"username\" and \"password\" keys for basic authentication", err) | ||
| } |
There was a problem hiding this comment.
I think there may be scenarios where BasicAuth is used to pass a token instead of a username/password; in these cases, either the username or the password may be empty.
So, we should probably key off both the username and password being blank here. (And, we should permit the secret to be created with blank values for either one of the fields.)
There was a problem hiding this comment.
The backend assumes that the values will be username and password and sets it the same in registry object in function applyBasicAuthFromUserCredentials.
I don't think just leaving it empty will make a token form of authentication work. If needed, then we need to implement an OAuth2 token exchange flow. A pre-existing token or an API key-style auth is not supported by the current code flow. We will need to take it up as a separate ticket.
(Not able to find the CoderabbitAI's comments for some reason.)
| if basicAuthSecretName == "" && (strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "unauthorized")) { | ||
| return nil, fmt.Errorf("error locating chart: %w; registry requires authentication - select a Secret with \"username\" and \"password\" keys for basic authentication", err) | ||
| } |
| client.ChartPathOptions.Version = chartInfo.Version | ||
| cp, err = client.ChartPathOptions.LocateChart(chartLocation, settings) | ||
| if err != nil { | ||
| if auth_secret == "" && (strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "unauthorized")) { |
There was a problem hiding this comment.
Should the comparison to "unauthorized" be case-blind?
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
This looks good to me once your resolve my previous questions about case-blind error checks and token authentications (but below is another suggestion).
| password, | ||
| }, | ||
| }); | ||
| const createdSecretName = secretName.trim(); |
There was a problem hiding this comment.
You reference this value twice before this definition. You should consider moving it up and using it at lines 43 & 69.
9 participants
Analysis / Root cause:
Kubernetes Secret object is used to store username and password for authentication when trying to create a Helm release. If a Helm release is created using a URL and selecting a particular secret for authentication, the users are unable to change the secrets during upgrade. This poses a problem if the secret's name has been changed or removed.
Solution description:
Create a dropdown where the secret can be entered during upgrade. If no secret is entered, it will go with the existing secret name if it exists, otherwise the new secret will override the old one.
Screenshots / screen recording:
https://docs.google.com/document/d/1LDRXScwr_jOnvdWU0siqW1gBMM8hrhm3D0oz831gUPA/edit?tab=t.mcv80hqh092h#heading=h.2jo1jtiyytic
Test setup:
An OCI registry and a Helm repository requiring authentication via username and password.
Reviewers and assignees:
Summary by CodeRabbit