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.

Sign up for GitHub

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

Jump to bottom

Add clarity to MVI timeouts #2410

Merged
merged 7 commits into from
Nov 12, 2018
Merged

Add clarity to MVI timeouts #2410

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89b07a1
Add clarity to MVI timeouts
annaswims Oct 31, 2018
00c82d4
Merge branch 'master' into mvi_timeout_refactor
annaswims Oct 31, 2018
c7a22ed
Add mvi config spec
annaswims Nov 2, 2018
4960dcf
Merge branch 'master' into mvi_timeout_refactor
saneshark Nov 5, 2018
dd296ec
Merge branch 'master' into mvi_timeout_refactor
annaswims Nov 5, 2018
30006e7
Merge branch 'master' into mvi_timeout_refactor
annaswims Nov 12, 2018
add16ee
Merge branch 'master' into mvi_timeout_refactor
annaswims Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/settings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,8 @@ mhv:
# Settings for Master Veteran Index
mvi:
url: http://ps-dev.commserv.healthevet.va.gov:8110/psim_webservice/IdMWebService
open_timeout: 2
timeout: 10
open_timeout: 15
timeout: 15
mock: false
processing_code: T
client_cert_path: /fake/client/cert/path
Expand Down
2 changes: 0 additions & 2 deletions config/settings/development.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
mvi:
url: http://ps-dev.commserv.healthevet.va.gov:8110/psim_webservice/IdMWebService
open_timeout: 2
timeout: 10
mock: true
processing_code: T
client_cert_path: config/certs/vetsgov-localhost.crt
Expand Down
15 changes: 4 additions & 11 deletions lib/mvi/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,13 @@

module MVI
class Configuration < Common::Client::Configuration::SOAP
# :nocov:
def self.default_mvi_open_timeout
Rails.logger.warn 'Settings.mvi.open_timeout not set, using default'
2
def self.open_timeout
Settings.mvi.open_timeout
end

def self.default_mvi_timeout
Rails.logger.warn 'Settings.mvi.timeout not set, using default'
10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind removing default like these, provided you can link to the devops tickets here in the PR so we can verify they are properly set. If they are not set for each environment there is a risk of blowing up one of those environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not set in ansible/deployment/config/vets-api/prod-settings.local.yml.j2 which becomes config/settings.local.yml. We're good because they're set in config/settings.yml in this repo, which doesn't get overridden.

def self.read_timeout
Settings.mvi.timeout
end
# :nocov:

OPEN_TIMEOUT = Settings.mvi.open_timeout&.to_i || default_mvi_open_timeout
TIMEOUT = Settings.mvi.timeout&.to_i || default_mvi_timeout

def self.ssl_cert_path
Settings.mvi.client_cert_path
Expand Down
17 changes: 8 additions & 9 deletions spec/lib/mvi/configuration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,18 @@
end
end

# TODO(knkski): These tests probably aren't doing anything useful.
describe '.default_mvi_open_timeout' do
context 'when Settings.mvi.open_timeout is not set' do
it 'should use the default' do
expect(MVI::Configuration::OPEN_TIMEOUT).to eq(2)
describe '.open_timeout' do
context 'when Settings.mvi.open_timeout is set' do
it 'should use the setting' do
expect(MVI::Configuration.instance.open_timeout).to eq(15)
end
end
end

describe '.default_mvi_timeout' do
context 'when Settings.mvi.timeout is not set' do
it 'should use the default' do
expect(MVI::Configuration::TIMEOUT).to eq(10)
describe '.read_timeout' do
context 'when Settings.mvi.timeout is set' do
it 'should use the setting' do
expect(MVI::Configuration.instance.read_timeout).to eq(15)
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i support removing these default ones, but can we instead replace them to ensure they have whatever is specified in Settings.yml... Ideally, be explicit in the spec.

ie. expect(MVI::Configuration.new.timeout).to eq(15) NOT expect(MVI::Configuration.new.timeout).to eq(Settings.mvi.readtimeout)

Expand Down