Skip to content

Commit

Permalink
[EmbeddedAnsible::Credential] use id for manager_ref
Browse files Browse the repository at this point in the history
The `manager_ref` column is used by the authentications table to
reference a external resource id for a given provider.  Not required for
all uses, but generally used by provider authentications so we have a
mapping to a unique id on the provider side when doing a refresh so
we aren't clobbering existing records, allowing add/update/delete
operations to function properly when doing a refresh.

With `EmbeddedAnsible` now not using ansible tower, this is no longer
needed by default, but there are certain places where it is still being
called, like `ServiceTemplateAnsiblePlaybook.build_parameter_list` via
the `native_ref` method.

With the change to `EmbeddedAnsible::AutomationManager::Credential`'s
`native_ref` method to include a `Integer` type cast that was ported over
from the `manageiq-provider-ansible_tower` repo (not part of the
original POC by Jason), this caused errors with `nil` `manager_ref` ids
when trying to create a new `ServiceTemplateAnsiblePlaybook` and using
the default credential.  The purpose of this type casting originally was
to enforce a validation that was expected on the Ansible Tower side that
required Integer values when communicating through it's API and
referencing primary keys:

https://github.com/ManageIQ/manageiq-providers-ansible_tower/blob/487b4aef/spec/support/ansible_shared/automation_manager/credential.rb#L8

Since `manager_ref` no longer has value to `EmbeddedAnsible`, as it
isn't tied to a tower instance, we can just assign the `id` value to
`manager_ref` on create to give it a unique value that can be used in
places where searching by `native_ref` or `manager_ref` is required.

Also, the uses of `native_ref` in our codebase overall are limited:

https://github.com/search?q=org%3AManageIQ+native_ref&type=Code

(10 code locations across the ManageIQ org at the time of writing)
  • Loading branch information
NickLaMuro committed Jun 20, 2019
1 parent dbd6f04 commit 73fa537
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ class ManageIQ::Providers::EmbeddedAnsible::AutomationManager::Credential < Mana
alias_attribute :manager_id, :resource_id
alias_attribute :manager, :resource

after_create :set_manager_ref

COMMON_ATTRIBUTES = {}.freeze
EXTRA_ATTRIBUTES = {}.freeze
API_ATTRIBUTES = COMMON_ATTRIBUTES.merge(EXTRA_ATTRIBUTES).freeze
Expand All @@ -23,4 +25,9 @@ def self.notify_on_provider_interaction?
def native_ref
Integer(manager_ref)
end

def set_manager_ref
self.manager_ref = self.id
save!
end
end
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
describe ManageIQ::Providers::EmbeddedAnsible::AutomationManager::Playbook do
let(:manager) { FactoryBot.create(:embedded_automation_manager_ansible) }
let(:auth_one) { FactoryBot.create(:embedded_ansible_credential, :manager_ref => '6') }
let(:auth_two) { FactoryBot.create(:embedded_ansible_credential, :manager_ref => '8') }
let(:auth_one) { FactoryBot.create(:embedded_ansible_credential) }
let(:auth_two) { FactoryBot.create(:embedded_ansible_credential) }
subject { FactoryBot.create(:embedded_playbook, :manager => manager) }

describe '#run' do
Expand All @@ -21,8 +21,8 @@
:extra_vars => '{"a":"x"}',
:playbook => subject.name,
:project => 'mref',
:credential => 6,
:vault_credential => 8
:credential => auth_one.id,
:vault_credential => auth_two.id
)

allow(subject).to receive(:configuration_script_source).and_return(double(:manager_ref => 'mref'))
Expand Down
12 changes: 6 additions & 6 deletions spec/models/service_template_ansible_playbook_spec.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
describe ServiceTemplateAnsiblePlaybook do
let(:user) { FactoryBot.create(:user_with_group) }
let(:auth_one) { FactoryBot.create(:embedded_ansible_credential, :manager_ref => '6') }
let(:auth_two) { FactoryBot.create(:embedded_ansible_credential, :manager_ref => '10') }
let(:auth_three) { FactoryBot.create(:embedded_ansible_credential, :manager_ref => '14') }
let(:auth_one) { FactoryBot.create(:embedded_ansible_credential) }
let(:auth_two) { FactoryBot.create(:embedded_ansible_credential) }
let(:auth_three) { FactoryBot.create(:embedded_ansible_credential) }

let(:script_source) { FactoryBot.create(:configuration_script_source, :manager => ems) }

Expand Down Expand Up @@ -135,9 +135,9 @@
:description => description,
:become_enabled => true,
:verbosity => 3,
:credential => 6,
:network_credential => 10,
:vault_credential => 14
:credential => auth_one.id,
:network_credential => auth_two.id,
:vault_credential => auth_three.id
)

expect(params.keys).to_not include(:extra_vars, :cloud_credentials)
Expand Down

0 comments on commit 73fa537

Please sign in to comment.