Backup fix helm upgrade secrets#16775
Closed
sowmya-sl wants to merge 4 commits into
Closed
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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
component/helm
sowmya-sl
force-pushed
the
backup-fix-helm-upgrade-secrets
branch
from
July 20, 2026 12:12
f19ce60 to
9ebc9e5
Compare
openshift-ci
Bot
added
the
needs-rebase
Contributor
WalkthroughThis PR normalizes "Helm Release"/"Helm Releases" casing to "Helm release"/"Helm releases" (and related "Helm Chart" casing) across UI strings, locales, models, and Cypress integration tests. Separately, it adds Basic authentication secret selection/creation for URL-based Helm chart installs/upgrades, spanning new frontend UI, form state, and backend Go error handling with an added ChangesHelm release/chart casing normalization
Basic authentication secret support for URL-based Helm installs/upgrades
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 13❌ Failed checks (3 warnings, 10 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…tching - Pass basicAuthSecretName through UpgradeReleaseAsync to support auth credentials during chart upgrades - Use __none__ sentinel to allow users to explicitly clear a secret - Preserve installation annotation on upgrade to maintain URL-install tracking across revisions - Return errors instead of logging on auth credential failures so the frontend can surface them - Add 401/unauthorized detection in GetChartFromURL, InstallChartFromURL, and UpgradeReleaseAsync with actionable error messages - Update handler signature, mock, and tests for the new parameter
…orms - Add HelmCreateBasicAuthSecretModal for creating kubernetes.io/basic-auth secrets with username/password from within the Helm forms - Add auth secret dropdown with 'None' and 'Create Secret' options to HelmInstallUpgradeForm, shown when upgrading a URL-installed chart - Detect URL-installed charts via 'installation' annotation and read initial auth secret from 'helm.openshift.io/auth-secret' annotation - Pass basicAuthSecretName through to the upgrade API payload - Use __none__ sentinel to allow explicitly clearing a previously set secret - Show warning when a previously referenced secret no longer exists - Add auth secret dropdown to HelmURLChartForm for URL-based chart installs
- Lowercase 'release' after 'Helm' throughout: 'Helm Release' is not a proper noun, so use 'Helm release' and 'Helm releases' consistently - Revert 'Helm Charts' back to 'Helm Chart(s)' to match existing convention across the codebase - Update integration tests, step definitions, feature files, source code, and i18n strings to reflect the casing changes
sowmya-sl
force-pushed
the
backup-fix-helm-upgrade-secrets
branch
from
July 20, 2026 12:14
9ebc9e5 to
e008fbb
Compare
openshift-ci
Bot
removed
the
needs-rebase
- 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
Co-authored-by: Cursor <cursoragent@cursor.com>
sowmya-sl
force-pushed
the
backup-fix-helm-upgrade-secrets
branch
from
July 20, 2026 12:15
e008fbb to
58858ef
Compare
![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: 1
🧹 Nitpick comments (3)
pkg/helm/handlers/handler_test.go (1)
140-148: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise and assert the new authentication parameter.
The handler fake accepts but ignores
basicAuthSecretName, while the auth-focused upgrade test always passes""; regressions in forwarding, explicit selection, or"__none__"clearing could therefore pass.
pkg/helm/handlers/handler_test.go#L140-L148: add an expected secret name and assert the received argument.pkg/helm/actions/upgrade_release_test.go#L778-L780: add explicit-secret and"__none__"cases, including annotation assertions.🤖 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 140 - 148, The fakeUpgradeReleaseAsync helper in pkg/helm/handlers/handler_test.go:140-148 must accept an expected basic-auth secret name and assert it matches the received basicAuthSecretName argument. Extend the authentication-focused cases in pkg/helm/actions/upgrade_release_test.go:778-780 to cover an explicitly selected secret and "__none__" clearing, asserting the resulting annotations for both cases.frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
onClosefires even on the successful create path.
closeModal(true)(used both for cancel and, indirectly, after a successful create at line 79) always callsonClose?.(). This conflates "dismissed without saving" with "saved successfully." It's harmless today since consumers only toggle a boolean in both callbacks, but any future consumer relying ononCloseto revert state would incorrectly fire on success too.🤖 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, Update closeModal to distinguish successful saves from dismissals: preserve the existing closeOverlay and blur behavior, but invoke onClose only for cancellation/regular dismissal, not when closeModal(true) is used after successful creation. Adjust the create-success call path and any needed argument semantics around closeModal so saved completion does not trigger onClose.frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx (1)
36-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated basic-auth secret dropdown wiring, including a collision-prone sentinel value. Both files independently reimplement the same state, constants, and handlers for the "select/create Basic auth secret" dropdown; the shared root cause is that this logic isn't factored into a single hook, and both copies bake in
CREATE_SECRET_KEY = 'create-secret', a value that could collide with a real user-created secret name (unlikeNONE_SECRET_KEY = '__none__').
frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx#L36-L63: extractisCreateSecretModalOpenstate, theCREATE_SECRET_KEY/NONE_SECRET_KEYconstants,handleSecretSave, andhandleSecretChangeinto a shared hook (e.g.useBasicAuthSecretField), and changeCREATE_SECRET_KEYto a value less likely to collide with real secret names.frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx#L77-L131: replace the duplicated block with the same shared hook, reusing the fixed, collision-safe sentinel constant.🤖 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/HelmURLChartForm.tsx` around lines 36 - 63, Extract the duplicated Basic auth secret dropdown state, constants, and handlers from HelmURLChartForm.tsx lines 36-63 and HelmInstallUpgradeForm.tsx lines 77-131 into a shared useBasicAuthSecretField hook, and replace both local implementations with that hook. Change CREATE_SECRET_KEY to a collision-safe sentinel distinct from real secret names, while preserving NONE_SECRET_KEY and existing modal/save behavior; both files must reuse the shared hook and sentinel.
🤖 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 `@pkg/helm/actions/upgrade_release.go`:
- Around line 263-265: Update the annotation-copy logic in the release upgrade
flow to avoid dereferencing rel.Chart.Metadata when it is nil. Reuse the
existing nil-safe metadata or annotation lookup used earlier, while preserving
the current behavior of copying the “installation” value into
ch.Metadata.Annotations when present.
---
Nitpick comments:
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx`:
- Around line 45-54: Update closeModal to distinguish successful saves from
dismissals: preserve the existing closeOverlay and blur behavior, but invoke
onClose only for cancellation/regular dismissal, not when closeModal(true) is
used after successful creation. Adjust the create-success call path and any
needed argument semantics around closeModal so saved completion does not trigger
onClose.
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx`:
- Around line 36-63: Extract the duplicated Basic auth secret dropdown state,
constants, and handlers from HelmURLChartForm.tsx lines 36-63 and
HelmInstallUpgradeForm.tsx lines 77-131 into a shared useBasicAuthSecretField
hook, and replace both local implementations with that hook. Change
CREATE_SECRET_KEY to a collision-safe sentinel distinct from real secret names,
while preserving NONE_SECRET_KEY and existing modal/save behavior; both files
must reuse the shared hook and sentinel.
In `@pkg/helm/handlers/handler_test.go`:
- Around line 140-148: The fakeUpgradeReleaseAsync helper in
pkg/helm/handlers/handler_test.go:140-148 must accept an expected basic-auth
secret name and assert it matches the received basicAuthSecretName argument.
Extend the authentication-focused cases in
pkg/helm/actions/upgrade_release_test.go:778-780 to cover an explicitly selected
secret and "__none__" clearing, asserting the resulting annotations for both
cases.
🪄 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: a63271f6-e0bb-4d0d-9eeb-c551bf918284
📒 Files selected for processing (48)
frontend/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-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/locales/en/helm-plugin.jsonfrontend/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/install-upgrade/HelmInstallUpgradePage.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/forms/url-chart/helm-oci-validation-utils.tsfrontend/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.tspkg/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
💤 Files with no reviewable changes (1)
- frontend/packages/helm-plugin/integration-tests/support/step-definitions/common/common.ts
Comment on lines
+263
to
+265
| if inst, ok := rel.Chart.Metadata.Annotations["installation"]; ok { | ||
| ch.Metadata.Annotations["installation"] = inst | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard previous chart metadata before copying annotations.
Line 263 dereferences rel.Chart.Metadata unconditionally, although the earlier logic explicitly treats that metadata as optional. A URL upgrade of a release with missing metadata can panic instead of returning an error; reuse a nil-safe annotation lookup 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 `@pkg/helm/actions/upgrade_release.go` around lines 263 - 265, Update the
annotation-copy logic in the release upgrade flow to avoid dereferencing
rel.Chart.Metadata when it is nil. Reuse the existing nil-safe metadata or
annotation lookup used earlier, while preserving the current behavior of copying
the “installation” value into ch.Metadata.Annotations when present.
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
New Features
Bug Fixes
Style