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

Accept write_timeout configuration, and supply it along to savoy #587

Merged

Conversation

nevinera
Copy link
Contributor

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.

@iloveitaly
Copy link
Member

looks great, thanks!

@iloveitaly iloveitaly merged commit a23f1f5 into NetSweet:master Aug 14, 2023
8 of 18 checks passed
iloveitaly added a commit that referenced this pull request Aug 14, 2023
@iloveitaly
Copy link
Member

@nevinera looks like CI was not passing on this, could you open up a new PR with all tests passing? I think you'll need to do a savon version check because not all versions support this option.

@nevinera
Copy link
Contributor Author

@iloveitaly - definitely, I didn't realize how far back the support reached. Looks like write_timeout was added in 2.13.0, I'll include a version check and enable the test-suite on my end.

nevinera added a commit to nevinera/netsuite that referenced this pull request Aug 15, 2023
@nevinera
Copy link
Contributor Author

nevinera commented Aug 15, 2023

@iloveitaly - what is the preferable experience for an engineer that attempts to set the write_timeout option while having a version of Savon installed that doesn't support it?

My instinct is to raise a custom exception like UnsupportedConfiguration explaining the issue as part of write_timeout=/write_timeout(value) - will that work for you? (I could use ConfigurationError, or I could subclass it for a more specific exception if you prefer).

@nevinera nevinera deleted the add-write-timeout-configuration branch August 15, 2023 15:24
iloveitaly pushed a commit that referenced this pull request Aug 21, 2023
* Revert "Revert "Accept write_timeout configuration, and supply it along to savoy (#587)""

This reverts commit f43ead8.

* Extract savon_params as a variable for manipulation

* extract Configuration#savon_params and test it directly

* Only supply and allow setting write_timeout when Savon supports it

* These tests need to execute against both old and new versions of Savon

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.
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