Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix trailing slash on Image Prefix #2747

Merged

Conversation

driverpt
Copy link
Contributor

@driverpt driverpt commented Aug 26, 2024

What does this PR do?

Fixes Trailing Slash on Image Prefix.

Why is it important?

Because we need to manually remove the Trailing Slash specifically for Go Projects. Where we want consistency between Testcontainers Java and Go

Related issues

How to test this PR

Unit Test Created

@driverpt driverpt requested a review from a team as a code owner August 26, 2024 08:51
Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 5be5853
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/66cc6edc0c41b90008a256c0
😎 Deploy Preview https://deploy-preview-2747--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya mdelapenya self-assigned this Aug 27, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Aug 27, 2024
@mdelapenya
Copy link
Member

Hi @driverpt thanks for contributing the fix! I'm curious about the use case: from what I read from your message, you have projects in Java and in Go, using testcontainers, right? Can you share more about the configuration used in both? I guess you're setting the hub prefix in the properties file, and because it's shared across languages, it fails for Go. Is this assumption correct?

@driverpt
Copy link
Contributor Author

driverpt commented Aug 27, 2024

Ok, so what happens is that we are using a CDK Template for CodeBuild PR Builders (Java, Go and Typescript), all of them have the same TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX but only Go fails because instead of handling the Trailing slash, it just blindly adds // if you have a Trailing Slash on that Variable.

This PR properly handles this via url.JoinPath() instead of blindly Concatenating 2 strings

@mdelapenya
Copy link
Member

Yeah, sure, I'm not questioning the fix: I added the test myself with the current head and I saw it failing, so thanks for contributing the test too!

I'm just interested in how testcontainers is used across a set multi-language projects.

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdelapenya mdelapenya merged commit 0d9e934 into testcontainers:main Aug 27, 2024
112 checks passed
@driverpt driverpt deleted the fix-trailing-slash-on-image-prefix branch August 27, 2024 14:05
mdelapenya added a commit that referenced this pull request Sep 3, 2024
* main:
  feat(wait): for file (#2731)
  feat(compose): select services via profiles (#2758)
  chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (#2761)
  fix: update template too (#2763)
  chore(deps): bump actions/checkout from 4.1.1 to 4.1.7 (#2762)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (#2760)
  fix: check if the discovered docker socket responds (#2741)
  Upgrade milvus-io/milvus-sdk-go to avoid checksum mismatch. (#2753)
  Fix trailing slash on Image Prefix (#2747)
  chore: use new testcontainers/ryuk:0.9.0 image (#2750)
  chore(deps): bump minimal Go version from 1.21 to 1.22 (#2743)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Sep 9, 2024
* main:
  docs: refine heading badges in README (testcontainers#2770)
  feat(wait): for file (testcontainers#2731)
  feat(compose): select services via profiles (testcontainers#2758)
  chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (testcontainers#2761)
  fix: update template too (testcontainers#2763)
  chore(deps): bump actions/checkout from 4.1.1 to 4.1.7 (testcontainers#2762)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (testcontainers#2760)
  fix: check if the discovered docker socket responds (testcontainers#2741)
  Upgrade milvus-io/milvus-sdk-go to avoid checksum mismatch. (testcontainers#2753)
  Fix trailing slash on Image Prefix (testcontainers#2747)
  chore: use new testcontainers/ryuk:0.9.0 image (testcontainers#2750)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Sep 11, 2024
* main:
  ci: add generate for mocks (testcontainers#2774)
  fix: docker config error handling when config file does not exist (testcontainers#2772)
  docs: refine heading badges in README (testcontainers#2770)
  feat(wait): for file (testcontainers#2731)
  feat(compose): select services via profiles (testcontainers#2758)
  chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (testcontainers#2761)
  fix: update template too (testcontainers#2763)
  chore(deps): bump actions/checkout from 4.1.1 to 4.1.7 (testcontainers#2762)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (testcontainers#2760)
  fix: check if the discovered docker socket responds (testcontainers#2741)
  Upgrade milvus-io/milvus-sdk-go to avoid checksum mismatch. (testcontainers#2753)
  Fix trailing slash on Image Prefix (testcontainers#2747)
  chore: use new testcontainers/ryuk:0.9.0 image (testcontainers#2750)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX trailing slash not handled properly
2 participants