OCPBUGS-99434: Show toast notification for invalid Helm Chart repositories#16792
OCPBUGS-99434: Show toast notification for invalid Helm Chart repositories#16792martinszuc wants to merge 7 commits into
Conversation
When a Helm Chart repository has incorrect basicAuth credentials or is unreachable, the backend already marks it in the index.yaml response via a console-warning annotation, but the frontend never read it. Backend: Improve error messages to distinguish auth failures (401/403) from other errors, and include the error reason in the annotation. Frontend: Read the console-warning annotation from the index.yaml response and display it as a toast notification on the catalog page. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
… support Surface both fetch-level errors (auth failures, timeouts) and config-level errors (missing secret keys, missing ConfigMaps, HTTPS required) in the console-warning annotation. Previously config errors were silently logged and the repo was dropped without user notification. Change List() to return config errors alongside valid repos so IndexFile() can merge them into the annotation. Use AlertVariant.danger (red) for the toast. Fix newline join to comma-space for proper HTML rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@martinszuc: This pull request references HELM-586 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHelm repository listing now returns structured per-repository errors. The chart proxy serializes these errors into a warning annotation, and the frontend parses the annotation to show a localized danger toast for unreachable repositories. ChangesHelm repository error reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HelmRepoGetter
participant ChartProxy
participant useHelmCharts
participant Toast
HelmRepoGetter->>ChartProxy: Return repositories and InvalidRepo entries
ChartProxy->>ChartProxy: JSON-marshal console-warning annotation
ChartProxy-->>useHelmCharts: Return chart index with console-warning
useHelmCharts->>Toast: Show localized danger notification
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
component/backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/chartproxy/repos_test.go`:
- Around line 303-313: The config-error test continues indexing configErrors
after reporting a length mismatch, which can panic when it is shorter than
expected. In the test loop around expectedConfigErrorRepo, return or otherwise
stop execution immediately after the len(configErrors) mismatch, while
preserving the existing per-repository assertions for matching lengths.
In `@pkg/helm/chartproxy/repos.go`:
- Around line 276-278: Preserve errors from metadata-name extraction in both
affected paths at pkg/helm/chartproxy/repos.go lines 276-278 and 293-295:
capture the error returned by unstructured.NestedString and incorporate it into
the InvalidRepo error instead of discarding it. Prefer a shared helper for
consistent handling across both sites, while retaining the extracted name when
available.
- Line 90: Update the deferred response-body cleanup in the relevant repository
request function to use a closure that checks the error returned by
resp.Body.Close() and logs any failure through the existing logging mechanism,
rather than discarding it. Preserve the current cleanup behavior and use the
function’s established logger.
🪄 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: c5faed9f-f128-49c6-9c7a-b593da53f112
📒 Files selected for processing (6)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsxpkg/helm/actions/utility.gopkg/helm/chartproxy/proxy.gopkg/helm/chartproxy/repos.gopkg/helm/chartproxy/repos_test.go
Use t.Fatalf for config error count mismatch to prevent index-out-of-bounds panic in subsequent loop. Log resp.Body.Close() errors instead of silently discarding them. Add fallback name for repos where metadata.name extraction fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helm/chartproxy/repos.go (1)
272-275: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate repository-listing failures.
These branches log dynamic-client errors but return
nil, making RBAC/API outages appear as successful empty or partial repository lists. Return the original error; reserve[]InvalidRepofor per-repository configuration failures.As per path instructions,
**/*.gorequires error returns not to be ignored.Also applies to: 292-295
🤖 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/chartproxy/repos.go` around lines 272 - 275, Update the repository-listing error branches in the function containing clusterRepos and the corresponding branch around the other repository list to return the original dynamic-client error instead of nil. Preserve logging and existing helmRepos/configErrors values, while reserving InvalidRepo entries for per-repository configuration failures.Source: Path instructions
🤖 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/chartproxy/repos.go`:
- Around line 90-94: Update the response-body close error handling in the
surrounding repository-fetch function to avoid logging indexURL verbatim, since
it may include basic-auth credentials from h.URL.User. Log a redacted URL or
only the safe host and path while preserving the existing close-error message
and behavior.
---
Outside diff comments:
In `@pkg/helm/chartproxy/repos.go`:
- Around line 272-275: Update the repository-listing error branches in the
function containing clusterRepos and the corresponding branch around the other
repository list to return the original dynamic-client error instead of nil.
Preserve logging and existing helmRepos/configErrors values, while reserving
InvalidRepo entries for per-repository configuration failures.
🪄 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: c30fb227-dab7-437c-830a-09e460494b7b
📒 Files selected for processing (2)
pkg/helm/chartproxy/repos.gopkg/helm/chartproxy/repos_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/helm/chartproxy/repos_test.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
|
/retest |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
This change is OK as it stands, but I think you should probably use a different approach to the i18n for the name: error strings. And, I've flagged several other items for your consideration.
/lgtm
| const repoList = repos | ||
| .map((r) => t('{{name}}: {{error}}', { name: r.name, error: r.error })) | ||
| .join(', '); |
There was a problem hiding this comment.
The translation key '{{name}}: {{error}}' seems odd and basically unwieldy. I don't think we can predict the value of name (that's basically user-supplied, right??), and therefore we cannot define a translation for this key before the fact (or, frankly, after the fact!).
Moreover, it's not obvious to me that internationalizing the name is possible or desirable. So, I think that we should be translating only the error, e.g.:
| const repoList = repos | |
| .map((r) => t('{{name}}: {{error}}', { name: r.name, error: r.error })) | |
| .join(', '); | |
| const repoList = repos | |
| .map((r) => r.name + ': ' + t(r.error)) | |
| .join(', '); |
And, of course, to make this work properly, we need to replace the proposed line 25 of frontend/packages/helm-plugin/locales/en/helm-plugin.json with instances of all of our favorite error messages.
| "Browse for charts that help manage complex installations and upgrades. Cluster administrators can customize the content made available in the catalog.<1></1>": "Browse for charts that help manage complex installations and upgrades. Cluster administrators can customize the content made available in the catalog.<1></1>", | ||
| "README": "README", | ||
| "README not available": "README not available", | ||
| "{{name}}: {{error}}": "{{name}}: {{error}}", |
There was a problem hiding this comment.
I'm not certain, but I think this entry is useless: my understanding is, the translation will apply to everything not inside {{...}}...which is just the colon (which requires no translation).
So, we can omit this from here, omit the t() call which references it, and just fill in the template.
Alternatively, see my comment below.
| annotations := make(map[string]string) | ||
| var invalidRepos []string | ||
| invalidRepos := append([]InvalidRepo{}, configErrors...) |
There was a problem hiding this comment.
Are you sure that you want to be making a copy of configErrors, here?
My instinct is that, if we're concerned about mutating the list that List() returned, then that function should have the responsibility of creating the copy (but I could be wrong...).
If we really want/need to make a copy, consider using Clone() instead:
invalidRepos := slices.Clone(configErrors)so that the intention is explicit.
| data, err := json.Marshal(invalidRepos) | ||
| if err == nil { | ||
| annotations[warning] = string(data) | ||
| indexFile.Annotations = annotations | ||
| } |
There was a problem hiding this comment.
If json.Marshal() returns an error here, we silently ignore it and move on...which is kind of unsatisfying.
If this is a "never happen" situation, then we should consider panicking if it does. Otherwise, maybe attach the error as an annotation, or log it, or somehow try to alert somebody that things aren't working?
| helmConfig, err := b.unmarshallConfig(item, "", true) | ||
| if err != nil { | ||
| repoName, _, _ := unstructured.NestedString(item.Object, "metadata", "name") |
There was a problem hiding this comment.
Are you sure you want to ignore the boolean and error return values? Maybe we don't gain enough by looking at them to justify the code and runtime cost...I don't know.
| for _, item := range namespaceRepos.Items { | ||
| helmConfig, err := b.unmarshallConfig(item, namespace, false) | ||
| if err != nil { | ||
| repoName, _, _ := unstructured.NestedString(item.Object, "metadata", "name") | ||
| if repoName == "" { | ||
| repoName = "<unknown>" | ||
| } | ||
| klog.Errorf("Error unmarshalling repo %v: %v", item, err) | ||
| configErrors = append(configErrors, InvalidRepo{Name: repoName, Error: err.Error()}) | ||
| continue | ||
| } | ||
| helmRepos = append(helmRepos, helmConfig) | ||
| } |
There was a problem hiding this comment.
This code looks ripe for refactoring: I think it's a near duplicate of lines 273-285 which I think can be moved into a helper function.
| if len(configErrors) != len(tt.expectedConfigErrorRepo) { | ||
| t.Fatalf("Expected %v config errors, but got %v", len(tt.expectedConfigErrorRepo), len(configErrors)) | ||
| } | ||
| for i, expectedName := range tt.expectedConfigErrorRepo { | ||
| if configErrors[i].Name != expectedName { | ||
| t.Errorf("Expected config error repo %v but got %v", expectedName, configErrors[i].Name) | ||
| } | ||
| if configErrors[i].Error == "" { | ||
| t.Errorf("Expected non-empty error message for repo %v", expectedName) | ||
| } | ||
| } |
There was a problem hiding this comment.
Not to disagree with Coderabbitai or anything, but, if this test fails because the counts don't match, the next question that a developer is going to ask is, "What was the mismatch?", and the code isn't going to offer any help. 🫤
Ideally, you would do a "set difference" operation (or two), expecting there to be no difference, and reporting any "extra" ones, either unexpectedly present or absent. Comparing the counts is a quick and easy way to do the check, but it doesn't reveal any helpful information about the failure, which means that the subsequent diagnosis requires reproducing the problem under the debugger, which is potentially cumbersome.
I'll also note that expecting the config errors to come out in a particular order seems fragile at best. (You've finessed this problem by ensuring that there is only ever one error, but you've set the test up to handle multiple...although you don't actually test that.)
I don't have any good suggestions, but I might be inclined to drop the initial count check and to have a nested loop which searches for each expected error in the actual list (instead of assuming and requiring that it show up at a particular index): that would enable reporting any expected ones which were missing. If you put that into a helper function, you could then call it again with the arguments reversed to report any actual ones which weren't in the expected list. This would yield full information without too much code, but it's probably more trouble than it's worth...especially if you only expect one error. 😉 Alternatively, after ensuring that the expected ones were present, you could then use the count-based check to ensure that there weren't any extras...which would at least give you half the information.
|
Scheduling tests matching the |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
…rage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Martin Szuc <mszuc@redhat.com>
martinszuc
changed the title
|
@martinszuc: This pull request references HELM-804 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
|
/retest |
martinszuc
changed the title
openshift-ci-robot
added
the
jira/valid-bug
|
@martinszuc: This pull request references Jira Issue OCPBUGS-99434, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
| if namespace != "" { | ||
| namespaceRepos, err := b.Client.Resource(helmChartRepositoryNamespaceGVK).Namespace(namespace).List(context.TODO(), v1.ListOptions{}) | ||
| if err != nil { | ||
| klog.Errorf("Error listing namespace helm chart repositories: %v \nempty repository list will be used", err) |
There was a problem hiding this comment.
It hardly matters, but, assuming that helmRepos contains entries from the Cluster/default namespace, it won't be empty when we return it in this error case.
In general, there's very little value in having the log message "predict" what will happen, because, when the log is read, it will have already happened. And, the developer can look at the code where the message was produced and see what it would have done.
So, the second half of this log message is pointless. If you have to make an edit for some other reason, feel free to remove it (here and at line 289, although that one, while also largely pointless, is at least accurate 🙂).
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: martinszuc, webbnh 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 |
|
@martinszuc: 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. |
3 participants
Analysis / Root cause:
Follow-up to PR #15624 (RFE-7965) which added basicAuth support for ProjectHelmChartRepository. That PR only covered the success case — when a user configures incorrect credentials or a misconfigured Secret (e.g., typo in key name), charts from the affected repository silently disappear from the Developer Catalog with no feedback.
Two gaps existed:
console-warningannotation on the aggregated index.yaml response for repos that failed to fetch, but the frontend never read it.Solution description:
Backend:
repos.go— 401/403 returns"authentication failed (HTTP %d)"instead of dumping the raw Go response struct. Other non-200 returns"failed to fetch index (HTTP %d)". Addeddefer resp.Body.Close()to fix a pre-existing resource leak.HelmRepoGetter.List()to return configuration-level errors (missing Secret keys, missing ConfigMaps, HTTPS required) alongside valid repos, instead of silently dropping misconfigured repos.console-warningannotation from a flat string to a JSON array of{name, error}objects to support frontend i18n.proxy.goto merge both config-level and fetch-level errors into a single annotation.Frontend:
useHelmCharts.tsxnow parses theconsole-warningJSON annotation and displays a danger toast notification viauseToast(). The toast appears when the entire Software Catalog is opened (not just the Helm Charts filter), since the index.yaml aggregation runs for all catalog types. Uses auseRefto prevent duplicate toasts on re-renders. Toast is dismissible and auto-dismisses after the default timeout.Tests:
TestHelmRepoGetter_ListErrorsto assert that misconfigured repos produce the correctconfigErrorswith expected repo names and non-empty error messages.Screenshots / screen recording:
Auth failure (wrong credentials) — toast on Software Catalog:
Auth failure — toast on Helm Charts catalog filter:
Unreachable repo — connection timeout error:
Config error (typo in Secret key name —
usenrameinstead ofusername):Test setup:
Test cases:
usenrameinstead ofusername)Browser conformance:
Additional info:
defer resp.Body.Close()fix addresses a pre-existing resource leak in the same functionconsole-warningannotation format changed from a flat string to JSON — the frontend handles both gracefully (malformed JSON is silently ignored via try/catch)Reviewers and assignees:
Summary by CodeRabbit