Conversation
gshikhar2021
commented
Apr 10, 2026
gshikhar2021
commented
- removed using grep command on logs
- Using exec command with timeout
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Ollama embedding service setup in the E2E tests by splitting model operations into separate commands with 10-minute timeouts. Feedback points out a compilation error involving io.ReadAll on a string type, recommends removing the unused io import, and suggests quoting a jsonpath expression for shell safety.
test/e2e/main_test.go
Outdated
| outBytes, err := io.ReadAll(p.Out()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read pod name output: %v", err) | ||
| } | ||
| ollamaPod := podNameOutput.Out() | ||
| podName := strings.TrimSpace(string(outBytes)) |
There was a problem hiding this comment.
The utils.RunCommand function from sigs.k8s.io/e2e-framework/pkg/utils returns a *CmdResult where Out() returns a string. Passing a string to io.ReadAll will cause a compilation error. Since the output is already a string, you can directly trim it.
| outBytes, err := io.ReadAll(p.Out()) | |
| if err != nil { | |
| return fmt.Errorf("failed to read pod name output: %v", err) | |
| } | |
| ollamaPod := podNameOutput.Out() | |
| podName := strings.TrimSpace(string(outBytes)) | |
| podName := strings.TrimSpace(p.Out()) |
test/e2e/main_test.go
Outdated
| import ( | ||
| "context" | ||
| "fmt" | ||
| "io" |
There was a problem hiding this comment.
The io package is no longer needed if the io.ReadAll call is removed as suggested below.
test/e2e/main_test.go
Outdated
| log.Printf("Failed to get Ollama pod name: %s", podNameOutput.Err()) | ||
| return podNameOutput.Err() | ||
| cmd := fmt.Sprintf( | ||
| "kubectl get pods -n %s -l app=ollama-embedding -o jsonpath={.items[0].metadata.name}", |
There was a problem hiding this comment.
It is recommended to wrap the jsonpath expression in single quotes to prevent potential shell expansion of the curly braces, especially since utils.RunCommand executes the command via a shell.
| "kubectl get pods -n %s -l app=ollama-embedding -o jsonpath={.items[0].metadata.name}", | |
| "kubectl get pods -n %s -l app=ollama-embedding -o jsonpath='{.items[0].metadata.name}'", |
- removed using grep command on logs - Using exec command with timeout
gshikhar2021
force-pushed
the
enhance-e2e
branch
from
845df5d to
269e11a
Compare
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) | ||
| defer cancel() | ||
|
|
||
| pullCmd := exec.CommandContext(ctx, "kubectl", "exec", "-n", testNamespace, podName, "--", "ollama", "pull", "nomic-embed-text:latest") |
There was a problem hiding this comment.
whats the difference between previous implementation and this implementation
2 participants
