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

use var TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE #407

Merged
merged 1 commit into from
May 31, 2022

Conversation

mniak
Copy link
Contributor

@mniak mniak commented Feb 21, 2022

No description provided.

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #407 (bd4c1b6) into master (c0c2f90) will increase coverage by 0.26%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
+ Coverage   65.05%   65.32%   +0.26%     
==========================================
  Files          19       19              
  Lines        1159     1165       +6     
==========================================
+ Hits          754      761       +7     
+ Misses        301      300       -1     
  Partials      104      104              
Impacted Files Coverage Δ
reaper.go 79.68% <85.71%> (+0.37%) ⬆️
docker.go 65.58% <0.00%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0c2f90...bd4c1b6. Read the comment docs.

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.

Thanks for this contribution @mniak! It LGTM

I'm postponing the merge until I get a deeper understanding of #414

@vincentfree
Copy link

Can't wait for this feature, I'm using lima on a M1 macbook, the default docker sock won't work out of the box so this would safe a lot of work and time helping others with the setup

@vincentfree
Copy link

Thanks for this contribution @mniak! It LGTM

I'm postponing the merge until I get a deeper understanding of #414

can't this ship without the rest of #414?

@kgalli
Copy link

kgalli commented May 31, 2022

@mdelapenya it seems #414 might take some time to have all the edge cases covered. As this is a relatively small change with manageable side effects do you still think this change should wait?

From my point of view there is a lot of value in this change already. Is there any particular info you need to reconsider?

@vincentfree
Copy link

@mdelapenya it seems #414 might take some time to have all the edge cases covered. As this is a relatively small change with manageable side effects do you still think this change should wait?

From my point of view there is a lot of value in this change already. Is there any particular info you need to reconsider?

I second that. This small feature will help with multiple non native docker backends. With lima serving my docker instances I can't use testcontainers until this is implemented.

@mdelapenya
Copy link
Member

Thanks all for the positive feedback in this PR. I think it's safe to merge it as is, and delegate the full podman compatibility to #414

@baez90 if you agree I'm merging this one after your 👍

@prskr
Copy link
Contributor

prskr commented May 31, 2022

Sure, makes sense I think and rebasing this should be straightforward

@mdelapenya mdelapenya merged commit b0e8ee3 into testcontainers:main May 31, 2022
@mdelapenya
Copy link
Member

Merged, thanks!

image

@vincentfree
Copy link

Can I already use this feature?

@prskr
Copy link
Contributor

prskr commented Jun 8, 2022

It's not released yet but you can use it if you update your dependency to latest main branch if you really want

@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants