fix: address review feedback

- Close log stream after reading (prevents connection/fd leak)
- Strengthen tests to assert on output headers rather than error paths
- Remove unused import

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
(cherry picked from commit 922558fc1a)

pkg/action/release_testing.go

@@ -170,6 +170,7 @@ func (r *ReleaseTesting) getContainerLogs(out io.Writer, client kubernetes.Inter
 
 		fmt.Fprintf(out, "POD LOGS: %s (%s)\n", podName, c.Name)
 		_, err = io.Copy(out, logReader)
+		logReader.Close()
 		fmt.Fprintln(out)
 		if err != nil {
 			errs = append(errs, fmt.Errorf("unable to write logs for pod %s, container %s: %w", podName, c.Name, err))

pkg/action/release_testing_test.go

@@ -22,7 +22,6 @@ import (
 	"errors"
 	"io"
 	"os"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -141,14 +140,10 @@ func TestGetContainerLogs_MultipleContainers(t *testing.T) {
 
 	var buf bytes.Buffer
 	err := rt.getContainerLogs(&buf, client, "test-pod")
-	// The fake client doesn't serve real log streams, so we expect
+	require.NoError(t, err)
-	// per-container errors rather than success, but critically it should
+	output := buf.String()
-	// NOT fail with "a container name must be specified".
+	assert.Contains(t, output, "POD LOGS: test-pod (main)")
-	if err != nil {
+	assert.Contains(t, output, "POD LOGS: test-pod (sidecar)")
-		assert.NotContains(t, err.Error(), "a container name must be specified")
-		assert.Contains(t, err.Error(), "container main")
-		assert.Contains(t, err.Error(), "container sidecar")
-	}
 }
 
 func TestGetContainerLogs_WithInitContainers(t *testing.T) {
@@ -172,11 +167,11 @@ func TestGetContainerLogs_WithInitContainers(t *testing.T) {
 
 	var buf bytes.Buffer
 	err := rt.getContainerLogs(&buf, client, "test-pod")
-	if err != nil {
+	require.NoError(t, err)
-		// Both init and regular containers should be attempted
+	output := buf.String()
-		assert.Contains(t, err.Error(), "container init-setup")
+	// Init containers should appear before regular containers
-		assert.Contains(t, err.Error(), "container main")
+	assert.Contains(t, output, "POD LOGS: test-pod (init-setup)")
-	}
+	assert.Contains(t, output, "POD LOGS: test-pod (main)")
 }
 
 func TestGetContainerLogs_PodNotFound(t *testing.T) {
@@ -189,7 +184,7 @@ func TestGetContainerLogs_PodNotFound(t *testing.T) {
 	assert.Contains(t, err.Error(), "unable to get pod nonexistent-pod")
 }
 
-func TestGetPodLogs_MultiContainerOutput(t *testing.T) {
+func TestGetContainerLogs_OutputHeaderFormat(t *testing.T) {
 	pod := &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "multi-test",
@@ -204,17 +199,12 @@ func TestGetPodLogs_MultiContainerOutput(t *testing.T) {
 	}
 
 	client := fakeclientset.NewClientset(pod)
-	rt := &ReleaseTesting{
+	rt := &ReleaseTesting{Namespace: "default"}
-		Namespace: "default",
-		Filters:   map[string][]string{},
-	}
 
-	// Call getContainerLogs directly to test output formatting
 	var buf bytes.Buffer
-	_ = rt.getContainerLogs(&buf, client, "multi-test")
+	err := rt.getContainerLogs(&buf, client, "multi-test")
+	require.NoError(t, err)
 	output := buf.String()
-	// Even if logs fail, check that header formatting uses container names
+	assert.Contains(t, output, "POD LOGS: multi-test (container-a)")
-	if len(output) > 0 {
+	assert.Contains(t, output, "POD LOGS: multi-test (container-b)")
-		assert.True(t, strings.Contains(output, "(container-a)") || strings.Contains(output, "(container-b)"))
-	}
 }