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

Add write timeout configuration (backwards compatible) #589

Conversation

nevinera
Copy link
Contributor

@nevinera nevinera commented Aug 15, 2023

The default timeout for reading from netsuite in the gem's configuration is 60 seconds, and the 'open' timeout can be set, but the 'write' timeout was not settable, and defaulted to 0.25 seconds.

Allow the write-timeout to be configured, and supply it to Savoy alongside the read and open timeouts.

(revert-revert from #587)

But this time, include a version-check - if Savon is below version 2.13.0, it doesn't accept write_timeout, and we should also complain (NetSuite::ConfigurationError if someone tries to set that option on this gem). And I enabled CI on my fork, instead of running tests locally - I hadn't realized how much of a matrix you were testing against, sorry about that!

The Configuration#connection method got unwieldy during that process (and was a bit undertested), so I extracted Configuration#savon_params as a method, tested it directly, and included contexts exercising the cases when Savon::VERSION was just before and just after the write_timeout feature was added.

Which means we can't actually instantiate the connection when
_pretending_ to have a recent savon version, or we'll trick our own
compatibility checks into passing Savon parameters it doesn't support.
@nevinera nevinera changed the title Add write timeout configuration backwards compatible Add write timeout configuration (backwards compatible) Aug 21, 2023
@iloveitaly iloveitaly merged commit 4bc9c23 into NetSweet:master Aug 21, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants