-
-
Notifications
You must be signed in to change notification settings - Fork 271
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
Revert proxy change #579
Revert proxy change #579
Conversation
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.
Mmm, it's not clear why this caused an issue. Can we keep the additional test cases here and maybe add some more? Then I'm happy to revert.
spec/netsuite/configuration_spec.rb
Outdated
@@ -519,21 +519,13 @@ | |||
expect(config.proxy).to be_nil | |||
end | |||
|
|||
it 'does not pass in nil proxy to savon' do |
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.
can we keep this test? this is an important case to cover
spec/netsuite/configuration_spec.rb
Outdated
connection = config.connection | ||
|
||
expect(connection.globals.include?(:proxy)).to eql(true) | ||
config.connection |
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.
I think we'd want to keep this case too
@andrewdicken-stripe friendly ping here! Would love to get this change in. |
@iloveitaly I made the change you requested, but for some reason the builds started failing. Not sure if there's an issue with the test suite because I had it working before and the changes that I made shouldn't have caused tall the issues I'm seeing. |
Reverts recent proxy related change, explained further in issue here: #578. I'm not exactly sure why the proxy change breaks things but this fix should revert the behavior to how it was before.