Pulls file credentials parsing out of Azure class #324
Conversation
lib/train/transports/azure.rb
Outdated
| @@ -38,7 +38,8 @@ def initialize(options) | |||
| @cache[:api_call] = {} | |||
|
|
|||
| if @options[:client_secret].nil? && @options[:client_id].nil? | |||
| parse_credentials_file | |||
| values = AzureHelpers::FileCredentials.parse(@options) | |||
| @options.merge!(values) | |||
There was a problem hiding this comment.
I'd skip the local variable and inline this if there's no conditional usage of it.
| subscription_id: subscription_id, | ||
| tenant_id: credentials[subscription_id]['tenant_id'], | ||
| client_id: credentials[subscription_id]['client_id'], | ||
| client_secret: credentials[subscription_id]['client_secret'] |
There was a problem hiding this comment.
Either these should be fetch operations or there should be a rescue NoMethodError statement for trying to call [] on nil. Unless you're really sure all those values exist before getting here. =^)
There was a problem hiding this comment.
credentials[subscription_id] should be safe. If we use either the 0 index method or the index based lookup the subscription id is read from the credentials file. If find the credentials with the subscription_id then we have a guard to ensure that section exists in the file.
There may be an edge case if the file has no entries. I can test that out.
| def validate! | ||
| if @credentials.sections.count > 1 | ||
| raise 'Multiple credentials detected, please set the AZURE_SUBSCRIPTION_ID environment variable.' | ||
| end |
There was a problem hiding this comment.
Personally I find using guard statements easier to scan.
def validate!
return if @credentials.section.count < 2
raise 'Multiple credentials detected, please set the AZURE_SUBSCRIPTION_ID environment variable.'
end| assert_equal('my_subscription_id', result[:subscription_id]) | ||
| end | ||
|
|
||
| it 'raises and error when no subscription id given and multiple entries' do |
This change pulls file credentials parsing out of Azure.rb. In extracting the file credentials parsing I refactored the code to introduce a strategy pattern around the different ways one may access credentials from the file. I introduced more unit testing for the extracted classes based on their public interfaces. This change also fixes an implementation error where asking for the credential at index 0, would return the last item in your credentials file. It will now raise an error that you should give a value greater than 0. Signed-off-by: David McCown <[email protected]>
Signed-off-by: David McCown <[email protected]>
dmccown
force-pushed
the
dmccown/refactor-azure-connector
branch
from
a4d4f47 to
7e2f80a
Compare
Signed-off-by: David McCown <[email protected]>
dmccown
force-pushed
the
dmccown/refactor-azure-connector
branch
from
be91498 to
b2273ea
Compare
Signed-off-by: David McCown <[email protected]>
Signed-off-by: David McCown <[email protected]>
Signed-off-by: David McCown <[email protected]>
This change pulls file credentials parsing out of Azure.rb. In
extracting the file credentials parsing I refactored the code to
introduce a strategy pattern around the different ways one may access
credentials from the file.
I introduced more unit testing for the extracted classes based on their
public interfaces.
This change also fixes an implementation error where asking for the
credential at index 0, would return the last item in your credentials
file. It will now raise an error that you should give a value greater
than 0. Credit to @Padgett for catching this bug!
Signed-off-by: David McCown [email protected]