fix: consume ImagePushResponse - #3739
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
Walkthrough
ChangesRegistry image push completion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
austindrenski
changed the title
ImagePushResponseImagePushResponse
8aa14ae to
93de899
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@modules/registry/registry.go`:
- Around line 203-209: Add a unit test around PushImage in registry.go to cover
push stream failure handling. Create TestPushImage_HandleStreamErrors that mocks
Docker client ImagePush and exercises the JSONMessages(ctx) iteration path in
both failure modes: a non-nil iterator error from the range loop and a non-nil
m.Error on a JSON message. Verify PushImage returns the wrapped ref-specific
error in each case, using the PushImage and JSONMessages symbols to target the
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55bfaf99-3946-41ea-bdee-554f2a0a409e
📒 Files selected for processing (1)
modules/registry/registry.go
| for m, err := range output.JSONMessages(ctx) { | ||
| if err != nil { | ||
| return fmt.Errorf("push image %q: %w", ref, err) | ||
| } | ||
| if err := m.Error; err != nil { | ||
| return fmt.Errorf("push image %q: %w", ref, err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate PushImage/ImagePush tests and check whether JSONMessages iterator
# errors and per-message push errors are covered.
rg -n -C4 --type go '\bPushImage\b|\bImagePush\b|JSONMessages|\.Error\b' .Repository: testcontainers/testcontainers-go
Length of output: 50389
🏁 Script executed:
cat -n modules/registry/registry_test.go | head -250Repository: testcontainers/testcontainers-go
Length of output: 9168
🏁 Script executed:
grep -n "PushImage" modules/registry/registry_test.goRepository: testcontainers/testcontainers-go
Length of output: 170
🏁 Script executed:
grep -rn "PushImage" modules/registry/Repository: testcontainers/testcontainers-go
Length of output: 781
🏁 Script executed:
cat -n modules/registry/examples_test.go | sed -n '178,230p'Repository: testcontainers/testcontainers-go
Length of output: 2016
Add coverage for push stream errors.
The PushImage implementation now iterates the push stream to detect errors. Currently, PushImage is only exercised in example code which assumes success. Add a unit test to explicitly verify that both the iterator error (range loop err) and the per-message m.Error are handled and returned.
func TestPushImage_HandleStreamErrors(t *testing.T) {
// TODO: Mock the Docker client ImagePush to return a stream that yields:
// 1. An iterator error (e.g., context cancellation).
// 2. An `m.Error` in the JSON message payload.
// Verify that PushImage returns the wrapped error in both cases.
// This confirms the silent-push regression is addressed.
}🤖 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 `@modules/registry/registry.go` around lines 203 - 209, Add a unit test around
PushImage in registry.go to cover push stream failure handling. Create
TestPushImage_HandleStreamErrors that mocks Docker client ImagePush and
exercises the JSONMessages(ctx) iteration path in both failure modes: a non-nil
iterator error from the range loop and a non-nil m.Error on a JSON message.
Verify PushImage returns the wrapped ref-specific error in each case, using the
PushImage and JSONMessages symbols to target the behavior.
There was a problem hiding this comment.
@austindrenski can you add the unit test, as suggested by the bot?
There was a problem hiding this comment.
@mdelapenya Agreed — adding that targeted coverage would be helpful here, especially for both JSONMessages(ctx) failure modes: iterator errors and per-message m.Error values.
2 participants
What does this PR do?
Fix to consume the image push response (mirrors existing handling for image pull response).
Why is it important?
Encountered an issue this afternoon where pushing to the testcontainers-hosted
registrysilently failed, leaving tests to hang on thec.ImageExists(...)check.I initially patched this locally just to bubble the error message, but surprisingly enough it also fixed the silent failure. I'm not sure if it's entirely deterministic (e.g., tcp backpressure?), or depends on the complexity of the image being pushed (in my case, pushing the
registry:latestimage into the testcontainers-hostedregistry:latestcontainer :D).This bug probably deserves more test coverage, but I'm short on time tonight, and wanted to at least get the patch posted before I lose track of it (c'est la vie).