-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
Throw the new DockerUnavailableException
when Docker is not available
#1308
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should discuss this in an issue. We can merge the PR, but I think these features would be better in Docker.DotNet. Things like detecting the container runtime, getting private registry credentials, etc. should be part of Docker.DotNet where more developers benefit from. This is one reason why we forked Docker.DotNet and use our own implementation. The Java's client has way more features and is much more developer-friendly than our .NET counterpart.
9fccae7
to
72e96c1
Compare
Absolutely. Could be challenging, though! |
72e96c1
to
579d530
Compare
IIRC, we did not throw an exception on purpose because either one of the other providers may find a container runtime, or the user can set one explicitly via Edit: This comment was intended for: #1263. However, I now see that you are catching the exception here. |
(because it is either not running or misconfigured) The `ArgumentException` was not the appropriate exception to throw since the root of the problem is not a bad argument but the state of the system which is wrong.
579d530
to
160d1e4
Compare
I've read your comment several times now, both in this context and in the context of #1263 but I still can't make sense of it. 😕 I mentioned #1263 in the original description of this pull request because by merging only this pull request, the error message would read like this:
If merging both this pull request and #1263, the error message would read like this, hinting that running OrbStack is likely to solve the issue.
|
What does this PR do?
This pull request introduces a new
DockerUnavailableException
which is thrown instead ofArgumentException
which was not the appropriate exception to throw since the root of the problem is not a bad argument but the state of the system which is wrong.The exception message has also been slightly improved.
Why is it important?
Unlike
ArgumentException
,DockerUnavailableException
can be caught and handled.Related issues
Once both this pull request and #1263 are merged, the error messages will be improved, giving a better hint to the user at what went wrong. Here are two sample messages from the new
DockerUnavailableException
.Running Testcontainers after quitting Docker Desktop:
Running Testcontainers after quitting OrbStack:
How to test this PR
Quit Docker Desktop or configure a bad endpoint then try to run a container. Observe the improved exception and error message.