feat: Ignore port-forwarding extra host in reuse hash#1689
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis PR adds a JSON converter ( ChangesRuntime ExtraHosts Serialization Filtering
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/Testcontainers/Configurations/Commons/JsonIgnoreRuntimeExtraHosts.cs`:
- Around line 18-21: The Read override in JsonIgnoreRuntimeExtraHosts calls
JsonSerializer.Deserialize<IEnumerable<string>>(ref reader) which can return
null for JSON null; update the Read method to capture the result in a local
variable, check for null and return Enumerable.Empty<string>() instead of
propagating null (ensure System.Linq is available), so the method always returns
a non-null IEnumerable<string>.
- Around line 23-27: The Write method in JsonIgnoreRuntimeExtraHosts currently
calls value.Where(...) which will throw if value is null; update
Write(Utf8JsonWriter writer, IEnumerable<string> value, JsonSerializerOptions
options) to guard for null by treating a null value as an empty sequence (e.g.,
use value ?? Enumerable.Empty<string>()), filter out any null elements and then
apply the StartsWith filter and OrderBy before serializing; reference the Write
method and PortForwardingHostEntry constant when making the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 28d6ecf0-fbb0-4431-97be-ec6fcb19573e
📒 Files selected for processing (3)
src/Testcontainers/Configurations/Commons/JsonIgnoreRuntimeExtraHosts.cssrc/Testcontainers/Configurations/Commons/JsonIgnoreRuntimeResourceLabels.cssrc/Testcontainers/Configurations/Containers/ContainerConfiguration.cs
What does this PR do?
The PR ignores the port-forwarding extra host value in the reuse hash. That value is set as soon as the port-forwarding container runs.
Why is it important?
TBH, I'm not sure this change is ideal. I wouldn't expect reuse + port-forwarding to work reliably anyway, because the port-forwarding container is currently a singleton and gets recreated for every test session (process). As a result, it ends up with different IP addresses, which leads to different container configurations and therefore different reuse hashes.
I believe ignoring it for now enables more use cases. Once we can reuse the port-forwarding container itself, this likely won't be necessary anymore.
Related issues
Summary by CodeRabbit
New Features
Documentation