Associate job with credentials #14113
Conversation
|
@miq-bot assign @gmcculloug |
|
@Fryguy @gtanzillo This looks good to me. Please review. |
| self.authentications = %w(credential_id cloud_credential_id network_credential_id).collect do |credential_attr| | ||
| credential_ref = raw_job.try(credential_attr) | ||
| next if credential_ref.blank? | ||
| ext_management_system.credentials.find_by(:manager_ref => credential_ref) |
There was a problem hiding this comment.
This is an N+1. I think this whole thing can be simplified to
credential_refs = %w(credential_id cloud_credential_id network_credential_id).collect { |attr| raw_job.try(attr) }.delete_blanks
self.authentications = ext_management_system.credentials.where(:manager_ref => credential_refs)Also, where did the credentials association come from. Is that new? cc @blomquisg
There was a problem hiding this comment.
The Ansible Job is not an object we collect through inventory, this object is captured as part of the service provisioning task. A Job has three possible credential relationships, machine, network and cloud.
There was a problem hiding this comment.
Yeah, I understand that...still not sure how that applies to my comment...
find_by in a loop is an N+1 and can be easily fixed with collecting the query criteria up front.
There was a problem hiding this comment.
I was addressing that the object is not so much under provider control so @blomquisg might be like ¯_(ツ)_/¯.
| @@ -65,6 +73,7 @@ | |||
| expect(subject.ems_ref).to eq(the_raw_job.id) | |||
| expect(subject.status).to eq(the_raw_job.status) | |||
| expect(subject.parameters.first).to have_attributes(:name => 'param1', :value => 'val1') | |||
| expect(subject.authentications).to eq([machine_credential, cloud_credential, network_credential]) | |||
There was a problem hiding this comment.
This should be match_array instead of eq. This way a change in order should not affect the test.
| expect(subject.ems_ref).to eq(the_raw_job.id) | ||
| expect(subject.status).to eq(the_raw_job.status) | ||
| expect(subject.parameters.first).to have_attributes(:name => 'param1', :value => 'val1') | ||
| expect(subject.authentications).to eq([machine_credential, cloud_credential, network_credential]) |
|
Checked commit bzwei@054598e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
This is done when sync an Ansible Tower job with VMDB.
https://www.pivotaltracker.com/story/show/140808769