Skip to content

Conversation

mdelapenya
Copy link
Member

@mdelapenya mdelapenya commented Oct 22, 2024

What does this PR do?

This PR removes all the prefixes for docker.io from the images used in 1) tests for the library, 2) modules

Why is it important?

It's better to ask for "nginx:latest" and let the engine deal with the image references than to enforce an invalid docker registry.

If we wanted to be explicit about official images, we should have used https://index.docker.io/v1/library/nginx:latest instead.

It also removes a bug when using Docker Desktop, as it considered those images were not accessible for the already used docker credentials.

Images used in the tests, and in the modules are using a wrong Docker image prefix: docker.io.

It's better to ask for "nginx:latest" and let the engine deal with the image references.
@mdelapenya mdelapenya requested a review from a team as a code owner October 22, 2024 14:30
@mdelapenya mdelapenya added the bug An issue with the library label Oct 22, 2024
@mdelapenya mdelapenya self-assigned this Oct 22, 2024
@mdelapenya mdelapenya requested a review from stevenh October 22, 2024 14:30
Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit f4d36c4
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6718cfcebe959100083f633c
😎 Deploy Preview https://deploy-preview-2846--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 changed the title fix: simplify full-qualified image names fix: simplify fully-qualified image names Oct 22, 2024
kiview
kiview previously approved these changes Oct 22, 2024
Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Just one issue and a few questions

@stevenh
Copy link
Contributor

stevenh commented Oct 22, 2024

Oh if the concern is about *.io going away what about https://index.docker.io/v1/?

@mdelapenya
Copy link
Member Author

Oh if the concern is about *.io going away what about https://index.docker.io/v1/?

No, because that's the correct index for the registry (Docker Hub)

@mdelapenya mdelapenya requested a review from stevenh October 23, 2024 05:12
Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looking good, just a few suggestions while you're making changes.

@mdelapenya mdelapenya merged commit 7ca3d8e into testcontainers:main Oct 23, 2024
121 of 122 checks passed
@mdelapenya mdelapenya deleted the simplify-image-names branch October 23, 2024 12:14
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Oct 28, 2024
* main:
  chore: use require.(No)Error(t,err) instead of t.Fatal(err) (testcontainers#2851)
  fix: simplify fully-qualified image names (testcontainers#2846)
mdelapenya added a commit that referenced this pull request Oct 28, 2024
* main:
  fix!: data races (#2843)
  fix: mongodb replicaset should work with auth (#2847)
  chore: use require.(No)Error(t,err) instead of t.Fatal(err) (#2851)
  fix: simplify fully-qualified image names (#2846)
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.

3 participants