-
Notifications
You must be signed in to change notification settings - Fork 61
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
bump fog openstack gem to new version 1.1.1 & enable application credential authentication #272
Conversation
@@ -778,6 +782,20 @@ def validate_options | |||
}, | |||
optional('agent') => Hash, | |||
} | |||
|
|||
unless ((username && api_key) || (application_credential_id && application_credential_secret)) || | |||
(username && api_key && application_credential_id && application_credential_secret) |
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.
What do we check with (username && api_key && application_credential_id && application_credential_secret) ?
Finally all of the properties must not be nil?
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.
We wanted to check that not all of these credentials are provided.
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.
But then it should be
unless ((username && api_key) || (application_credential_id && application_credential_secret)) &&
!(username && api_key && application_credential_id && application_credential_secret)
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.
Agree, may be splitting this into two boolean variables make it more readable?
e.g.: using_api_key = username && api_key?
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.
please check above comment.
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.
please check above comment.
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.
LGTM
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.
Looks good now!
5429e79
to
f416866
Compare
No description provided.