fix: fetch logs from all containers in test pods - #32099
Conversation
When a test pod contains multiple containers (e.g. Istio/Consul/Vault sidecars), 'helm test --logs' failed with 'a container name must be specified'. This happened because GetPodLogs called the Kubernetes log API without specifying a container name. The fix fetches the pod spec first, then iterates over all containers (init containers + regular containers) and requests logs for each one explicitly. Errors from individual containers are collected and returned together via errors.Join rather than aborting on the first failure. Also fixes a typo: hooksByWight -> hooksByWeight. Closes helm#6902 Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
|
Promptless prepared a documentation update related to this change. Triggered by helm/helm#32099 Added a "Viewing Test Logs" section to |
There was a problem hiding this comment.
Pull request overview
This PR updates helm test --logs to fetch pod logs per container instead of relying on the pod-level default, so multi-container test pods no longer fail when Kubernetes requires an explicit container name.
Changes:
- Refactors
ReleaseTesting.GetPodLogsto fetch the pod first and iterate containers when collecting logs. - Adds joined per-container error handling and includes the container name in log headers.
- Expands
release_testing_test.gowith coverage for multi-container and init-container cases, plus a typo fix in a local variable name.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/action/release_testing.go |
Implements per-container pod log retrieval and aggregates container-level failures. |
pkg/action/release_testing_test.go |
Adds tests for multi-container/init-container log collection paths and updates an error assertion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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>
|
@sabre1041 @joejulian @TerryHowe — this PR addresses #6902 (and #9892) by fetching logs from all containers in test pods, which you discussed in PR #12113. Key differences from #12113 and #31854:
Would appreciate a review when you get a chance. Thanks! |
|
This was missed in the |
What this PR does / why we need it:
When a test pod contains multiple containers (e.g. Istio/Consul/Vault sidecars, or multi-container test setups),
helm test --logsfails with:This is because
GetPodLogs()called the Kubernetes log API without specifying a container name. When a pod has >1 container, the API requires an explicit container name.Fix: Fetch the pod spec first, then iterate over all containers (init containers + regular containers) and request logs for each one by name. This matches the behavior of
kubectl logs --all-containers.errors.Join()rather than aborting on the first failurePOD LOGS: <pod> (<container>)for clarityhooksByWight->hooksByWeightCloses #6902
Also addresses #9892
Special notes for your reviewer:
GetPodLogs()signature(container: X)suffix in the log header. Since multi-container pods previously always errored, there is no breaking change.If applicable: