Various improvements related to waitContainer #79
Conversation
| -- | ||
| -- __NOTE__: this endpoint will not return a response until the container | ||
| -- has stopped. This function may therefore fail with a timeout error if | ||
| -- the timeout is configured incorrectly in the HTTP manager. |
There was a problem hiding this comment.
The timeout isn't necessarily "configured incorrectly", but simply configured to be shorter than the container's execution time, right? It seems like that might be intended behavior from the HTTP manager configuration.
| -- | Use 'httpHandler' with 'defaultUnixManagerSettings' @unixSocketPath@ as | ||
| -- argument as an alternative to 'unixHttpHandler' that lets you customise | ||
| -- the settings of the 'HTTP.ManagerSettings' value that is returned. | ||
| defaultUnixManagerSettings :: FilePath -- ^ The socket to connect to | ||
| -> HTTP.ManagerSettings | ||
| defaultUnixManagerSettings fp = defaultManagerSettings { | ||
| managerRawConnection = return $ openUnixSocket fp | ||
| } where openUnixSocket filePath _ _ _ = do | ||
| s <- S.socket S.AF_UNIX S.Stream S.defaultProtocol | ||
| S.connect s (S.SockAddrUnix filePath) | ||
| makeConnection (SBS.recv s 8096) | ||
| (SBS.sendAll s) | ||
| (S.close s) | ||
|
|
There was a problem hiding this comment.
This is superb, and I wonder if it should get pushed upstream?
src/Docker/Client/Internal.hs
Outdated
| getEndpointTimeout :: Endpoint -> HTTP.ResponseTimeout | ||
| getEndpointTimeout (WaitContainerEndpoint _) = HTTP.responseTimeoutNone | ||
| getEndpointTimeout _ = HTTP.responseTimeoutDefault |
There was a problem hiding this comment.
Wait, doesn't this render the NOTE above irrelevant?
There was a problem hiding this comment.
Thanks for reviewing this! The third commit does make the NOTE from the first irrelevant, but it is a question of whether you want getEndpointTimeout in the code or not. Having it always forces no timeout for the endpoint and no way for the user of the library to override this. Specifying different ManagerSettings and not merging the third commit allows users to specify a timeout themselves, but it is then shared between all endpoints.
Personally, I have been using a version of this library with my changes in production for the past year and the approach with getEndpointTimeout is fine since you can always wrap the call to waitContainer in a timeout yourself. Happy to remove the NOTE if you are happy to merge everything otherwise.
|
Hi @denibertovic and @jprider63 👋🏻 this PR has been open for nearly two years now and I would still like to see something like this merged. Is there anything I can do to help this along? Do you need any help maintaining this library? |
|
@mbg I'll take a look over the weekend and see if I can get this merged. |
|
This is awesome! |
|
@mbg Finally got around to this. I don't have any particular strong opinion about this. But as you say if it works for you in practice for a while now I see no reason why we can't merge it. Thanks! |
|
Thanks for merging this.
…On Tue, Mar 22, 2022, 00:16 Hai Nguyen Quang ***@***.***> wrote:
This is awesome!
—
Reply to this email directly, view it on GitHub
<#79 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAE5V6WCYFUXFRVS2W5QHTVBFCQTANCNFSM4MBZAQBQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
This PR consists of three commits which address #78 in various ways:
waitContainer's documentationdefaultUnixManagerSettings. This is just some code refactored out fromunixHttpHandlerso thatunixHttpHandler "/var/run/docker.sock"is equivalent to:This allows the manager to be customised, e.g. to set a different timeout:
getEndpointTimeoutwhich returnsresponseTimeoutDefaultfor all endpoints exceptWaitContainerEndpointfor which it returnsresponseTimeoutNone. This is then used inmkHttpRequestto set the value ofresponseTimeoutfor the request that is constructed.I would think that the first two are fairly safe to merge, while the third one may require some thought.