fix: use path.Join instead of url.JoinPath when prepending a custom registry to an image#3308
Conversation
…custom registry to an image (testcontainers#3306)
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughReplaced use of net/url's JoinPath with path.Join for image substitutors, removed related error handling and net/url import, and added a test verifying substitution when the registry prefix includes a port (e.g., "my-registry:5000/foo:latest"). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Substitutor
Note right of Substitutor: Old flow (url.JoinPath)\ncould drop tag when prefix has port
Caller->>Substitutor: Substitute(prefix, image)
alt old (url.JoinPath)
Substitutor-->>Substitutor: url.Parse(prefix) (may treat as opaque -> drop second arg)
Substitutor-->>Caller: joinedPathOrEmpty + error handling
else new (path.Join)
Substitutor-->>Substitutor: path.Join(prefix, image)
Substitutor-->>Caller: "prefix/image" (nil error)
end
Note left of Caller: Test validates "my-registry:5000/foo:latest"
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, thanks for the detailed report, and for contributing a fix. 🙇
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
|
Merged, thank you so much for your work here 🙇 |
What does this PR do?
This MR swaps
url.JoinPathwithpath.Joinin image substitutors.Why is it important?
url.JoinPathusesurl.Parseinternally, which doesn't correctly parse URLs likeexample.com:443without a schema. Docker image reference cannot have a schema [docs]. Incorrectly parsed URL insideurl.JoinPathcauses the image tag (second argument) to be completely ignored [go playground], which results in 404 errors when pulling an image.Related issues
How to test this PR
I've added a test case to
image_substitutors_test.go:go test -v -run TestPrependHubRegistrySubstitutor