feat: add operations for helm - #17
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds Helm support to the Kubernetes extension: three new operations (kubernetes.helmInstall, kubernetes.helmList, kubernetes.helmUninstall), their handler implementations and unit tests, documentation updates, a CI step to install Helm, and a new Extension field to persist kubeconfigPath. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Extension
participant Shell as "Helm CLI (shell)"
participant Parser as "JSON Parser"
Client->>Extension: OperationRequest (helmInstall / helmList / helmUninstall)
Extension->>Extension: validate args, build helm command
Extension->>Shell: execute helm command
Shell-->>Extension: stdout/stderr, exit status
alt helmList (JSON output)
Extension->>Parser: parse JSON output
Parser-->>Extension: structured releases
end
Extension-->>Client: OperationResult (Success/Failure + output)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Fix all issues with AI agents
In `@pkg/extension/helm_test.go`:
- Around line 129-188: The test assumes the helm CLI exists; mirror the
TestHandleHelmList fix by detecting helm with exec.LookPath("helm") at the start
of TestHandleHelmUninstall and, if helm is not found, adjust the expectations
for cases that relied on the "release not found" behavior (e.g., the "valid name
parameter for non-existent release" and "with namespace" cases) or skip those
subtests—modify the tests in TestHandleHelmUninstall (referencing the
TestHandleHelmUninstall function and the ext.handleHelmUninstall call) so they
either set wantSuccess=false when helm is missing or call t.Skipf with a clear
message when the helm binary is absent.
- Around line 65-71: The test in helm_test.go currently skips asserting the
error text: when tt.wantErrMsg != "" it only verifies result.Success is false
but never checks result.Message content; update the test to import "strings"
and, inside the block where tt.wantErrMsg != "" && result.Message != "", assert
that strings.Contains(result.Message, tt.wantErrMsg) (and if not, call t.Errorf
with a helpful message), and also ensure you still fail if result.Success is
true when an error was expected.
- Around line 76-127: Test failures occur because the CI has no helm binary;
update the TestHandleHelmList test to detect helm availability and skip the test
when absent: in pkg/extension/helm_test.go inside TestHandleHelmList (or before
running subtests) call exec.LookPath("helm") (or run a simple
exec.CommandContext check) and if it returns an error call t.Skipf("skipping
helm-dependent tests: %v", err); this keeps handleHelmList and Extension
unchanged and avoids asserting wantSuccess=true when the helm executable is not
present.
In `@pkg/extension/helm.go`:
- Around line 44-47: The loop building cmdArgs with "--set" is vulnerable to
shell/Helm parsing problems and doesn't support nested maps; instead either
serialize complex structures to a temporary YAML file and pass it via "--values"
or implement a recursive flattener (e.g., flattenValues(prefix string, values
map[string]interface{}, result map[string]string)) to produce dot-notated keys
(service.type=LoadBalancer) and then append safe, properly formatted values to
cmdArgs; additionally ensure values are sanitized/quoted (use fmt.Sprintf or
strconv.Quote-like formatting) when constructing the final "--set" arguments to
avoid injection/escaping issues.
In `@README.md`:
- Around line 216-217: The README claim that helmList outputs a `releases` field
is inconsistent with the implementation: update the helmList implementation in
helm.go (the function/method named helmList that currently calls sdk.Success
with a formatted string) to return structured data (e.g., an object/struct with
a `releases` array of {name, namespace, status, chart} entries) via the SDK
success path instead of a formatted string, or alternatively update README.md to
describe the actual string returned by sdk.Success; pick one approach and make
the code/docs consistent by changing either helmList/sdk.Success usage or the
README description accordingly.
🧹 Nitpick comments (3)
pkg/extension/helm.go (3)
55-63: Consider sanitizing or quoting command output in error messages.The error message includes raw command output which could contain sensitive information (registry credentials, internal paths, etc.) that might be logged or displayed to users.
105-111: JSON unmarshal may fail on valid empty array output.If
helm listreturns an empty JSON array[],len(output) > 0is true, butreleaseswill be an empty slice after unmarshal, which is handled correctly at line 113. However, if the output is whitespace-only or contains just a newline, the unmarshal will fail.Consider trimming the output before the length check.
Suggested improvement
// Parse JSON output var releases []map[string]interface{} - if len(output) > 0 { + trimmedOutput := strings.TrimSpace(string(output)) + if len(trimmedOutput) > 0 { - if err := json.Unmarshal(output, &releases); err != nil { + if err := json.Unmarshal([]byte(trimmedOutput), &releases); err != nil { return sdk.Failure(fmt.Errorf("failed to parse helm list output: %s", err)), nil } }
157-164: Fragile "not found" detection.The string match
strings.Contains(string(output), "not found")is locale-dependent and may break with different Helm versions or localized error messages. Additionally, if the helm binary itself is not found, this check won't match and the error will propagate differently.Consider checking the exit code or using
helm statusfirst to verify release existence.
Comment on lines
+65
to
+71
| if tt.wantErrMsg != "" && result.Message != "" { | ||
| // Check if error message contains expected substring | ||
| // (we don't check exact match because helm error messages may vary) | ||
| if result.Success { | ||
| t.Errorf("expected failure with message containing %q, but got success", tt.wantErrMsg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Error message assertion is ineffective.
The check at lines 65-71 verifies tt.wantErrMsg but only checks if the result succeeded when an error message was expected. It never actually validates that result.Message contains the expected error substring.
Proposed fix
- if tt.wantErrMsg != "" && result.Message != "" {
- // Check if error message contains expected substring
- // (we don't check exact match because helm error messages may vary)
- if result.Success {
- t.Errorf("expected failure with message containing %q, but got success", tt.wantErrMsg)
- }
- }
+ if tt.wantErrMsg != "" {
+ if result.Success {
+ t.Errorf("expected failure with message containing %q, but got success", tt.wantErrMsg)
+ } else if !strings.Contains(result.Message, tt.wantErrMsg) {
+ t.Errorf("expected message containing %q, got %q", tt.wantErrMsg, result.Message)
+ }
+ }Note: You'll need to add "strings" to the imports.
🤖 Prompt for AI Agents
In `@pkg/extension/helm_test.go` around lines 65 - 71, The test in helm_test.go
currently skips asserting the error text: when tt.wantErrMsg != "" it only
verifies result.Success is false but never checks result.Message content; update
the test to import "strings" and, inside the block where tt.wantErrMsg != "" &&
result.Message != "", assert that strings.Contains(result.Message,
tt.wantErrMsg) (and if not, call t.Errorf with a helpful message), and also
ensure you still fail if result.Success is true when an error was expected.
Comment on lines
+44
to
+47
| // Add values as --set flags | ||
| for k, v := range values { | ||
| cmdArgs = append(cmdArgs, "--set", fmt.Sprintf("%s=%v", k, v)) | ||
| } |
There was a problem hiding this comment.
Command injection risk and limited values support.
Two concerns with the --set flag handling:
-
Shell metacharacters: If
korvcontain special characters (e.g., commas, brackets, quotes), they can break Helm's--setparsing or cause unexpected behavior. -
Nested values limitation: The current approach doesn't handle nested values. For example,
{"service": {"type": "LoadBalancer"}}would need--set service.type=LoadBalancer, not--set service=map[type:LoadBalancer].
Consider using a temporary values file with --values for complex structures, or recursively flatten nested maps with dot notation.
Sketch: Flatten nested values
func flattenValues(prefix string, values map[string]interface{}, result map[string]string) {
for k, v := range values {
key := k
if prefix != "" {
key = prefix + "." + k
}
switch val := v.(type) {
case map[string]interface{}:
flattenValues(key, val, result)
default:
result[key] = fmt.Sprintf("%v", val)
}
}
}🤖 Prompt for AI Agents
In `@pkg/extension/helm.go` around lines 44 - 47, The loop building cmdArgs with
"--set" is vulnerable to shell/Helm parsing problems and doesn't support nested
maps; instead either serialize complex structures to a temporary YAML file and
pass it via "--values" or implement a recursive flattener (e.g.,
flattenValues(prefix string, values map[string]interface{}, result
map[string]string)) to produce dot-notated keys (service.type=LoadBalancer) and
then append safe, properly formatted values to cmdArgs; additionally ensure
values are sanitized/quoted (use fmt.Sprintf or strconv.Quote-like formatting)
when constructing the final "--set" arguments to avoid injection/escaping
issues.
Comment on lines
+216
to
+217
| **Outputs:** | ||
| - `releases`: Information about found Helm releases (name, namespace, status, chart) |
There was a problem hiding this comment.
Documentation inconsistency: releases output doesn't match implementation.
The documentation states that helmList outputs a releases field, but the implementation in helm.go returns a formatted string message via sdk.Success(), not structured data with a releases key.
Either update the documentation to reflect the actual string output, or modify the implementation to return structured release data.
🤖 Prompt for AI Agents
In `@README.md` around lines 216 - 217, The README claim that helmList outputs a
`releases` field is inconsistent with the implementation: update the helmList
implementation in helm.go (the function/method named helmList that currently
calls sdk.Success with a formatted string) to return structured data (e.g., an
object/struct with a `releases` array of {name, namespace, status, chart}
entries) via the SDK success path instead of a formatted string, or
alternatively update README.md to describe the actual string returned by
sdk.Success; pick one approach and make the code/docs consistent by changing
either helmList/sdk.Success usage or the README description accordingly.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test.yaml:
- Around line 19-22: The workflow currently sets the Helm installer to use
`version: 'latest'` (in the azure/setup-helm@v4 step), which makes CI
non-deterministic; update the Helm setup step to pin a specific, tested release
(e.g., replace 'latest' with a concrete tag such as 'v4.1.0' for Helm v4 or
'v3.20.0' for Helm v3) so every run installs the exact same Helm version for
reproducible CI builds and local debugging.
Comment on lines
+19
to
+22
| - name: Install Helm | ||
| uses: azure/setup-helm@v4 | ||
| with: | ||
| version: 'latest' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest stable version of Helm 2025 2026
💡 Result:
As of February 9, 2026, the latest stable Helm releases are:
- Helm v4 (current major): v4.1.0 — released January 21, 2026. [1]
- Helm v3 (still maintained): v3.20.0 — released January 21, 2026. [2]
(Helm v4 was officially announced/released on November 12, 2025.) [3]
Sources:
[1] (github.com)
[2] (github.com)
[3] (cncf.io)
Citations:
- 1: https://github.com/helm/helm/releases
- 2: https://github.com/helm/helm/releases
- 3: https://www.cncf.io/announcements/2025/11/12/helm-marks-10-years-with-release-of-version-4/?utm_source=openai
🏁 Script executed:
# Check the actual workflow file for context
cat -n .github/workflows/test.yaml | sed -n '1,30p'Repository: mcpchecker/kubernetes-extension
Length of output: 653
🌐 Web query:
azure/setup-helm action latest version v4 v5
💡 Result:
- Latest
azure/setup-helmrelease/tag:v4.3.1(github.com) - Latest major version: v4 (you can use
azure/setup-helm@v4to track the latest v4.x). (github.com) - Is there a v5? Not currently—there’s no
v5release/tag in the upstream repo releases. (github.com)
Citations:
- 1: https://github.com/azure/setup-helm/releases?utm_source=openai
- 2: https://github.com/marketplace/actions/helm-tool-installer?utm_source=openai
- 3: https://github.com/azure/setup-helm/releases?utm_source=openai
Pin Helm to a specific version for reproducible CI builds.
Using version: 'latest' introduces non-determinism—different workflow runs may install different Helm versions, potentially causing unexpected test failures when a new Helm release ships breaking changes. Pin to a specific version (e.g., v4.1.0 for Helm v4, or v3.20.0 for Helm v3) to ensure reproducibility and easier local debugging.
♻️ Suggested fix
- name: Install Helm
uses: azure/setup-helm@v4
with:
- version: 'latest'
+ version: 'v4.1.0'🤖 Prompt for AI Agents
In @.github/workflows/test.yaml around lines 19 - 22, The workflow currently
sets the Helm installer to use `version: 'latest'` (in the azure/setup-helm@v4
step), which makes CI non-deterministic; update the Helm setup step to pin a
specific, tested release (e.g., replace 'latest' with a concrete tag such as
'v4.1.0' for Helm v4 or 'v3.20.0' for Helm v3) so every run installs the exact
same Helm version for reproducible CI builds and local debugging.
Cali0707
reviewed
Feb 10, 2026
Comment on lines
+32
to
+43
| if name != "" { | ||
| cmdArgs = append(cmdArgs, name) | ||
| } else { | ||
| cmdArgs = append(cmdArgs, "--generate-name") | ||
| } | ||
|
|
||
| cmdArgs = append(cmdArgs, chart) | ||
|
|
||
| if namespace != "" { | ||
| cmdArgs = append(cmdArgs, "--namespace", namespace) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
here let's also set the --kubeconfig flag - the extension can receive a different kubeconfig path than the default on startup.
We don't currently expose the path to that file through the client, but it should be simple to add something around here:
kubernetes-extension/pkg/extension/client.go
Lines 18 to 51 in 194d786
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
matzew
force-pushed
the
Kube_helm_extension
branch
from
February 27, 2026 08:19
4d46898 to
ba32a57
Compare
Contributor
Author
|
@Cali0707 updated |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/extension/helm.go (1)
59-60: Consider checking for helm binary availability.If the
helmbinary is not installed or not in PATH, the error message fromexec.CommandContextmay be cryptic. A pre-check withexec.LookPath("helm")and a clearer error message would improve user experience.💡 Suggested improvement
// Could add at the start of each handler or as a shared check: if _, err := exec.LookPath("helm"); err != nil { return sdk.Failure(fmt.Errorf("helm CLI not found in PATH: please install helm")), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/extension/helm.go` around lines 59 - 60, Add a pre-check for the helm binary before calling exec.CommandContext by using exec.LookPath("helm") where you construct cmd := exec.CommandContext(ctx, "helm", cmdArgs...) and return a clear sdk.Failure error if not found (e.g., "helm CLI not found in PATH: please install helm"); this check can be placed at the start of the handler or factored into a shared helper used by the code that runs CombinedOutput() so callers receive a human-friendly message instead of a cryptic exec error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/extension/helm_test.go`:
- Around line 18-26: The kubernetesAvailable function has inverted logic: when
cmd.CombinedOutput() returns an error it currently returns true for
non-"unreachable" errors; change it so any error from exec.Command("helm",
"list") causes kubernetesAvailable to return false (i.e., treat failures like
auth/permission errors as Kubernetes not available). Update the error branch in
kubernetesAvailable (which uses cmd.CombinedOutput() and strings.Contains) to
return false on err != nil, only returning true when the command succeeds.
---
Nitpick comments:
In `@pkg/extension/helm.go`:
- Around line 59-60: Add a pre-check for the helm binary before calling
exec.CommandContext by using exec.LookPath("helm") where you construct cmd :=
exec.CommandContext(ctx, "helm", cmdArgs...) and return a clear sdk.Failure
error if not found (e.g., "helm CLI not found in PATH: please install helm");
this check can be placed at the start of the handler or factored into a shared
helper used by the code that runs CombinedOutput() so callers receive a
human-friendly message instead of a cryptic exec error.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/test.yamlREADME.mdpkg/extension/extension.gopkg/extension/helm.gopkg/extension/helm_test.gopkg/extension/operations.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/extension/operations.go
- .github/workflows/test.yaml
- README.md
Comment on lines
+18
to
+26
| // kubernetesAvailable checks if a Kubernetes cluster is reachable via helm | ||
| func kubernetesAvailable() bool { | ||
| cmd := exec.Command("helm", "list") | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return !strings.Contains(string(output), "unreachable") | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Inverted logic in kubernetesAvailable check.
The logic at line 23 is inverted. If helm list fails with an error that doesn't contain "unreachable", the function returns true, incorrectly indicating Kubernetes is available. For example, authentication failures or permission errors would cause the function to return true.
🐛 Proposed fix
func kubernetesAvailable() bool {
cmd := exec.Command("helm", "list")
output, err := cmd.CombinedOutput()
if err != nil {
- return !strings.Contains(string(output), "unreachable")
+ // If helm list fails for any reason, assume Kubernetes is not available
+ return false
}
return true
}📝 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
| // kubernetesAvailable checks if a Kubernetes cluster is reachable via helm | |
| func kubernetesAvailable() bool { | |
| cmd := exec.Command("helm", "list") | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return !strings.Contains(string(output), "unreachable") | |
| } | |
| return true | |
| } | |
| // kubernetesAvailable checks if a Kubernetes cluster is reachable via helm | |
| func kubernetesAvailable() bool { | |
| cmd := exec.Command("helm", "list") | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| // If helm list fails for any reason, assume Kubernetes is not available | |
| return false | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/extension/helm_test.go` around lines 18 - 26, The kubernetesAvailable
function has inverted logic: when cmd.CombinedOutput() returns an error it
currently returns true for non-"unreachable" errors; change it so any error from
exec.Command("helm", "list") causes kubernetesAvailable to return false (i.e.,
treat failures like auth/permission errors as Kubernetes not available). Update
the error branch in kubernetesAvailable (which uses cmd.CombinedOutput() and
strings.Contains) to return false on err != nil, only returning true when the
command succeeds.
Cali0707
approved these changes
Feb 27, 2026
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
2 participants
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.
adding operations for working with helm charts
Summary by CodeRabbit
New Features
Documentation
Tests
Chores