Repository navigation
Conversation
…-py client library to http+docker://localnpipe. Other pipe-like URIs are rewritten this way too. So, the code should detect the http+docker scheme instead of "npipe" and return localhost. Because the check for "http" schemes will inadvertently pick up the http+docker scheme too, do the check for pipes first and for real URLs second.
| if 'http' in url.scheme or 'tcp' in url.scheme: | ||
| return url.hostname | ||
| if 'unix' in url.scheme or 'npipe' in url.scheme: | ||
| if 'unix' in url.scheme or 'http+docker' == url.scheme: |
Contributor
There was a problem hiding this comment.
Should npipe also be included in this check for backwards compatibility?
Author
|
That’s a good idea. I don’t know when the docker client started replacing npipe with the special URL.
… On Jun 19, 2020, at 2:39 AM, Till Hoffmann ***@***.***> wrote:
@tillahoffmann commented on this pull request.
In testcontainers/core/docker_client.py:
> @@ -62,11 +62,12 @@ def host(self):
except ValueError:
return None
- if 'http' in url.scheme or 'tcp' in url.scheme:
- return url.hostname
- if 'unix' in url.scheme or 'npipe' in url.scheme:
+ if 'unix' in url.scheme or 'http+docker' == url.scheme:
Should npipe also be included in this check for backwards compatibility?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Author
|
I updated the pull request with the npipe check. |
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
==========================================
+ Coverage 80.61% 81.93% +1.32%
==========================================
Files 20 20
Lines 459 476 +17
Branches 34 35 +1
==========================================
+ Hits 370 390 +20
+ Misses 70 66 -4
- Partials 19 20 +1
Continue to review full report at Codecov.
|
* adding kafka containers support * Removing the assertions inside the for loop as they are not guarenteed to provide correct feedback
| port = nginx.port_to_expose | ||
| url = "http://{}:{}/".format(nginx.get_container_host_ip(), | ||
| nginx.get_exposed_port(port)) | ||
| time.sleep(5) |
Contributor
There was a problem hiding this comment.
Can we drop the sleeps?
Author
|
I have those in my copy because the tests fail on my hardware without them, but I didn’t mean for them to end up in my pull request. Sorry about that, I’ll move them so they aren’t in the pull request.
…Sent from my iPad
On Mar 29, 2021, at 5:14 AM, Till Hoffmann ***@***.***> wrote:
@tillahoffmann commented on this pull request.
In tests/test_nginx_container.py:
> @@ -9,6 +10,7 @@ def test_docker_run_nginx():
port = nginx.port_to_expose
url = "http://{}:{}/".format(nginx.get_container_host_ip(),
nginx.get_exposed_port(port))
+ time.sleep(5)
Can we drop the sleeps?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
* adding support for passing custom env-file (testcontainers#134) * adding support for passing env-file to DockerCompose (testcontainers#134) * fixed flake8 * trigger Gihub Actions again Co-authored-by: Anton Petrosyuk <[email protected]>
…-py client library to http+docker://localnpipe. Other pipe-like URIs are rewritten this way too. So, the code should detect the http+docker scheme instead of "npipe" and return localhost. Because the check for "http" schemes will inadvertently pick up the http+docker scheme too, do the check for pipes first and for real URLs second.
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
==========================================
+ Coverage 80.61% 81.93% +1.32%
==========================================
Files 20 20
Lines 459 476 +17
Branches 34 35 +1
==========================================
+ Hits 370 390 +20
+ Misses 70 66 -4
- Partials 19 20 +1
Continue to review full report at Codecov.
|
…ws that was causing them.
Member
|
what was the end outcome here? |
This was referenced Mar 8, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Windows npipe URIs are rewritten by the docker-py client library to http+docker://localnpipe. Other pipe-like URIs are rewritten this way too. So,
core/docker_client.pyshould detect the http+docker scheme instead of "npipe" and return localhost. Because the check for "http" schemes will inadvertently pick up the http+docker scheme too, do the check for pipes first and for real URLs second. Fixes issue #99. All tests pass with this fix on Windows 10 with Docker Desktop running locally.