Automate method to list ansible credentials #53
Conversation
|
@bzwei @tinaafitz |
|
@mkanoor Looks good. |
| dialog_hash = { | ||
| 'sort_by' => "value", | ||
| 'data_type' => "string", | ||
| 'required' => true, |
There was a problem hiding this comment.
credential is optional since it is already predefined.
There was a problem hiding this comment.
@bzwei
I dont understand what the required means in the context of dialogs. I will ask @eclarizio
| 'required' => true, | ||
| 'sort_order' => "ascending", | ||
| 'values' => list, | ||
| 'default_value' => list.length == 1 ? list.keys.first : nil |
There was a problem hiding this comment.
The list should be like
nil => '<Default>'
id => 'credential name'
default will be always nil => <Default>, which means to use the one already defined in the job template.
|
|
||
| def credentials | ||
| result = provider.try(:credentials) || [] | ||
| result.select { |c| c.type == @handle.inputs['credential_type'] } |
There was a problem hiding this comment.
you can use provider.credentials.find_by(:type => type). How does inputs['credential_type'] get set?
There was a problem hiding this comment.
@bzwei
The credential_type is set as an input parameter into the method. So the 2 instances available_network_credential will set
credential_type =
ManageIQ::Providers::AnsibleTower::AutomationManager::NetworkCredential
available_machine_credentials will set
credential_type =
ManageIQ::Providers::AnsibleTower::AutomationManager::MachineCredential
The method itself has a default credential_type set to
ManageIQ::Providers::AnsibleTower::AutomationManager::MachineCredential
There was a problem hiding this comment.
@bzwei
The find_by works on associations, by the time we get the credentials on the Automate method side it has been converted into an array.
There was a problem hiding this comment.
@handle.vmdb('Authentication').where("resource_id = ? AND type = ?",provider.id, @handle.inputs['credential_type'])
This would work do you prefer this
There was a problem hiding this comment.
Thanks for the explanation. From performance point of view, this is better.
| # for provisioning we use service_template | ||
| # for reconfig and retire use service | ||
| service = @handle.root['service_template'] || @handle.root['service'] | ||
| service.try(:job_template, 'Provision').try(:manager) |
There was a problem hiding this comment.
@mkanoor It is possible that the different actions could run JobTemplates on different providers. We would need to determine the service_action here and not just hardcode Provision.
There was a problem hiding this comment.
@gmcculloug I discussed this with @bzwei and this is where we came up with the the restriction that all job templates in a service have to point to the same manager. Since credentials are connected to a manager for the entire job template we will have a single manager. So all service_actions (Provision, Reconfigure, Retire) will point to the same manager and hence the same set of credentials.
|
|
||
| def fetch_list_data | ||
| credential_list = Hash[*credentials.pluck(:id, :name).flatten] | ||
| @handle.log(:info, "Number of credentials found #{credential_list.keys.count}") |
There was a problem hiding this comment.
Any reason why credential_list.keys.count and not just credential_list.count?
There was a problem hiding this comment.
@gmcculloug didn't realize Hash has a count method
There was a problem hiding this comment.
Good for debugging, but I am not convinced we want to log the count here in final release.
| 'required' => false, | ||
| 'sort_order' => "ascending", | ||
| 'values' => list, | ||
| 'default_value' => nil |
There was a problem hiding this comment.
sort_by should be "description" and since the value is an ID data_type should be "integer".
| datatype: | ||
| priority: 1 | ||
| owner: | ||
| default_value: ManageIQ::Providers::AnsibleTower::AutomationManager::MachineCredential |
There was a problem hiding this comment.
Can default_value be blank since the instance is always passing this parameter?
There was a problem hiding this comment.
@gmcculloug
One reason I decided to do that was to preserve the expected format of the value when the method gets copied around to a different class/domain without instances.
|
|
||
| def fetch_list_data | ||
| credential_list = Hash[*credentials.pluck(:id, :name).flatten] | ||
| @handle.log(:info, "Number of credentials found #{credential_list.keys.count}") |
There was a problem hiding this comment.
Good for debugging, but I am not convinced we want to log the count here in final release.
| def fetch_list_data | ||
| credential_list = Hash[*credentials.pluck(:id, :name).flatten] | ||
| @handle.log(:info, "Number of credentials found #{credential_list.keys.count}") | ||
| return nil => "<none>" if credential_list.blank? |
There was a problem hiding this comment.
I would simply do
def fetch_list_data
credential_list = Hash[*credentials.pluck(:id, :name).flatten]
{nil => '<Default>'}.merge(credential_list)
end
|
@bzwei @gmcculloug |
|
Checked commits mkanoor/manageiq-content@607e6f7~...6693372 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
https://www.pivotaltracker.com/story/show/137304515
Automate method to list machine, network and other ansible credentials.
The method accepts a type and yields a
{id => name} hash
e.g.
{id1 => "test1", id2 => "test2"}
Depends on PR ManageIQ/manageiq#13912