Prepare parameter hash before passing to Tower API credential CU #14483

jameswnl · 2017-03-23T22:42:36Z

convert :userid to :username
add :kind
add :organization => 1 if it's the EmbeddedAnsible. I.e. assign to Default organization in Tower

jameswnl · 2017-03-23T22:43:05Z

@miq-bot add_labels wip, enhancement, providers/ansible_provider

miq-bot · 2017-03-23T22:43:11Z

@jameswnl Cannot apply the following label because they are not recognized: providers/ansible_provider

jameswnl · 2017-03-23T22:44:05Z

@miq-bot add_labels providers/ansible_tower

jameswnl · 2017-03-23T23:45:03Z

@miq-bot remove_labels wip

bdunne · 2017-03-24T13:37:12Z

spec/support/ansible_shared/automation_manager/credential.rb

  let(:finished_task) { FactoryGirl.create(:miq_task, :state => "Finished") }
-  let(:manager)       { FactoryGirl.create(:provider_ansible_tower, :with_authentication).managers.first }
+  let(:manager)       { FactoryGirl.create(ansible_provider, :with_authentication).managers.first }


I think it would be much simpler to define a let for the provider outside of the shared examples?

So I tried it (defining outside), but it is failing somehow. To succeed, I'll have to pass it implicitly.

I feel having the shared_examples parameters being explicitly declared in the block opening (as in shared_examples_for do | ansible_provider | ) provides more clarity.

It works here... #14419

Yeah, but as I said, that is passing the parameter implicitly and I prefer more explicit passing which brings more clarity to the code.

bdunne · 2017-03-24T13:38:40Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb

@@ -75,4 +83,5 @@ def delete_in_provider_queue
  COMMON_ATTRIBUTES = {}.freeze
  EXTRA_ATTRIBUTES = {}.freeze
  API_ATTRIBUTES = COMMON_ATTRIBUTES.merge(EXTRA_ATTRIBUTES).freeze
+  # TOWER_KIND = 'ssh'.freeze # default to `ssh` just like Tower does


Why is this commented?

i am not sure if we want/need this default at all. what do you think?

This type should never be created, right? So I wouldn't include the constant here.

Right. I had this line started out for helping the test (as it was originally at the Ansible::Credential level. yeah, now removed.

bdunne · 2017-03-24T20:07:05Z

app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb

  include ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::Credential
+
+  def self.provider_params(params)
+    super.merge(:organization => 1)


Is it safe to hard code this id?

See my new patch. Thanks Brandon!

Not blocking the PR for the spec

mzazrivec · 2017-03-27T13:29:46Z

With the changes from this PR in place, I still cannot update userid for a machine credential via api:

$ curl -X PUT -d '{"userid":"user01"}' http://admin:smartvm@localhost:3000/api/authentications/10000000000184
...

Not when I look at the task id returned:

$ curl http://admin:smartvm@localhost:3000/api/tasks/10000000111645|json_reformat
{
    "href": "http://localhost:3000/api/tasks/10000000111645",
    "id": 10000000111645,
    "name": "Updating ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential with manager_ref=45",
    "state": "Finished",
    "status": "Error",
    "message": "undefined method `userid=' for #<AnsibleTowerClient::Credential:0x0055efef731808>\nDid you mean?  username=",
    "userid": "system",
    "created_on": "2017-03-27T13:27:56Z",
    "updated_on": "2017-03-27T13:28:00Z"
}

jameswnl · 2017-03-27T14:33:20Z

@mzazrivec I just did one with my setup and can't reproduce yours.

jameswnl · 2017-03-27T14:34:46Z

I just pushed an update to have EmbeddedAnsible to always augment the parameters being passed to Tower to have :organization_id => provider.default_organization.

jameswnl · 2017-03-27T16:15:57Z

Found the cause to @mzazrivec 's issue. the userid being passed in is string while backend code is expecting :userid - a symbol. Now backend will work on both.

bdunne · 2017-03-27T16:14:51Z

spec/models/manageiq/providers/embedded_ansible/automation_manager/credential_spec.rb

@@ -1,5 +1,9 @@
 require 'support/ansible_shared/automation_manager/credential'

 describe ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential do
-  it_behaves_like 'ansible credential', :provider_embedded_ansible
+  let(:ansible_manager) do
+    FactoryGirl.create(:provider_embedded_ansible, :with_authentication, :with_default_organization).managers.first


I don't think it's reusable enough to justify adding a trait to the factory, this should work:

FactoryGirl.create(:provider_embedded_ansible, :with_authentication, :default_organization => 1).managers.first

Great! Good to know. Done now

bdunne · 2017-03-27T16:20:20Z

spec/support/ansible_shared/automation_manager/credential.rb

  let(:finished_task) { FactoryGirl.create(:miq_task, :state => "Finished") }
-  let(:manager)       { FactoryGirl.create(ansible_provider, :with_authentication).managers.first }
+  let(:manager)       { ansible_manager }


You don't need to do this. You can drop this line if you rename here and here.

bdunne · 2017-03-27T16:21:15Z

spec/factories/provider.rb

@@ -23,6 +23,11 @@
        x.authentications << FactoryGirl.create(:authentication, :status => "Valid")
      end
    end
+    trait(:with_default_organization) do


Please remove. See comment below

…by default

miq-bot · 2017-03-27T17:12:28Z

Checked commits jameswnl/manageiq@5346f5c~...de286f8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 7 offenses detected

spec/support/ansible_shared/automation_manager/credential.rb

❗ - Line 22, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 23, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 29, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 30, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 31, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 32, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 33, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

bdunne · 2017-03-27T17:22:23Z

spec/models/manageiq/providers/ansible_tower/automation_manager/credential_spec.rb

@@ -1,5 +1,9 @@
 require 'support/ansible_shared/automation_manager/credential'

-describe ManageIQ::Providers::AnsibleTower::AutomationManager::Credential do
+describe ManageIQ::Providers::AnsibleTower::AutomationManager::ScmCredential do


Why is credential_spec.rb testing ScmCredential?

Because ManageIQ::Providers::AnsibleTower::AutomationManager::Credential is not a concrete class and doesn't have TOWER_KIND.

Maybe I'll rename the file to scm_credential_spec.rb?

bdunne · 2017-03-27T17:23:33Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb

@@ -2,7 +2,15 @@ module ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::Credential
  extend ActiveSupport::Concern

  module ClassMethods
+    def provider_params(params)
+      params[:username] = params.delete(:userid) if params.include?(:userid)
+      params[:username] = params.delete('userid') if params.include?('userid')


Why both a String and Symbol key for userid?

string type is coming from API calls via HTTP.

You want me to take away the line handling :userid? I keep it there so it will be consistent with all other attributes as in here

Other usage through ruby method invocation can use whole set of attributes using symbol key.

or else will have to change all the credentials' ATTRIBUTES hash keys.

ManageIQ#14483 (comment)

The code is not needed since ManageIQ/manageiq#14483 has been merged.

ManageIQ#14483 (comment)

The code is not needed since ManageIQ/manageiq#14483 has been merged.

martinpovolny · 2017-04-04T06:11:18Z

UI ManageIQ/manageiq-ui-classic#818 is fine/yes so this has to be too.

simaishi · 2017-04-04T14:29:23Z

@jameswnl Can you take a look at the fine branch and see if there is anything that need to be backported from this PR? It seems backporting other PR(s) already put all needed changes to fine branch. After resolving conflicts, I ended up with just 1 file changed, which is just to add extra spaces.

+++ b/spec/support/ansible_shared/automation_manager/credential.rb
@@ -17,10 +17,10 @@ shared_examples_for "ansible credential" do
 
     let(:params) do
       {
-        :description => "Description",
-        :name        => "My Credential",
-        :related     => {},
-        :userid      => 'john'
+        :description  => "Description",
+        :name         => "My Credential",
+        :related      => {},
+        :userid       => 'john'
       }
     end

jameswnl · 2017-04-04T16:20:44Z

@simaishi fine branch looks fine to me :)

simaishi · 2017-04-04T16:30:18Z

Thanks @jameswnl - marking it as fine/backported

miq-bot changed the title ~~Prepare parameter hash before passing to Tower API credential CU~~ [WIP] Prepare parameter hash before passing to Tower API credential CU Mar 23, 2017

miq-bot added enhancement wip labels Mar 23, 2017

miq-bot added the providers/ansible_tower label Mar 23, 2017

jameswnl force-pushed the massage-cred branch from 4687d12 to 3b643f0 Compare March 23, 2017 23:28

jameswnl changed the title ~~[WIP] Prepare parameter hash before passing to Tower API credential CU~~ Prepare parameter hash before passing to Tower API credential CU Mar 23, 2017

miq-bot removed the wip label Mar 23, 2017

bdunne previously requested changes Mar 24, 2017

View reviewed changes

chessbyte assigned blomquisg Mar 24, 2017

mzazrivec mentioned this pull request Mar 24, 2017

CRUD for embedded Ansible credentials ManageIQ/manageiq-ui-classic#641

Merged

jameswnl force-pushed the massage-cred branch 2 times, most recently from b5640bb to 22fba33 Compare March 24, 2017 15:57

bdunne reviewed Mar 24, 2017

View reviewed changes

bdunne added the euwe/no label Mar 27, 2017

jameswnl force-pushed the massage-cred branch from 4fc22b4 to 221f49a Compare March 27, 2017 14:27

jameswnl force-pushed the massage-cred branch from 221f49a to 31d99eb Compare March 27, 2017 16:07

bdunne requested changes Mar 27, 2017

View reviewed changes

jameswnl force-pushed the massage-cred branch from 31d99eb to 2da39bf Compare March 27, 2017 16:33

jameswnl added 3 commits March 27, 2017 13:06

parameterize credential spec for in/ext ansible

5346f5c

Tower credential userid => username before API call

7cf4f55

Tower credential fill in kind attribute before API call

f0245a2

jameswnl added 2 commits March 27, 2017 13:06

EmbeddedAnsible credential create/update will set :organization => 1 …

e83d01c

…by default

Use embedded ansible's default_organization

de286f8

jameswnl force-pushed the massage-cred branch from 2da39bf to de286f8 Compare March 27, 2017 17:06

bdunne requested changes Mar 27, 2017

View reviewed changes

bdunne approved these changes Mar 27, 2017

View reviewed changes

bdunne merged commit 4a645a4 into ManageIQ:master Mar 27, 2017

bdunne added a commit to bdunne/manageiq that referenced this pull request Mar 27, 2017

Fix Rubocop alignment complaints

c56500f

ManageIQ#14483 (comment)

bdunne mentioned this pull request Mar 27, 2017

Fix Rubocop alignment complaints #14527

Merged

bdunne added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 27, 2017

chessbyte assigned bdunne and unassigned blomquisg Mar 27, 2017

mzazrivec added a commit to mzazrivec/manageiq-ui-classic that referenced this pull request Mar 28, 2017

Remove unneeded code from ansible credentials controller

a66b602

The code is not needed since ManageIQ/manageiq#14483 has been merged.

mzazrivec mentioned this pull request Mar 28, 2017

Error when creating machine credential (embedded ansible) #14532

Closed

odravison pushed a commit to odravison/manageiq that referenced this pull request Mar 28, 2017

Fix Rubocop alignment complaints

ab42a4d

ManageIQ#14483 (comment)

mzazrivec added a commit to mzazrivec/manageiq-ui-classic that referenced this pull request Mar 29, 2017

Remove unneeded code from ansible credentials controller

daed36c

The code is not needed since ManageIQ/manageiq#14483 has been merged.

jameswnl deleted the massage-cred branch March 29, 2017 14:34

mzazrivec mentioned this pull request Mar 30, 2017

Remove unneeded code from ansible credentials controller ManageIQ/manageiq-ui-classic#818

Merged

martinpovolny added the fine/yes label Apr 4, 2017

simaishi added the fine/conflict label Apr 4, 2017

simaishi added fine/backported and removed fine/conflict labels Apr 4, 2017

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare parameter hash before passing to Tower API credential CU #14483

Prepare parameter hash before passing to Tower API credential CU #14483

jameswnl commented Mar 23, 2017

jameswnl commented Mar 23, 2017

miq-bot commented Mar 23, 2017

jameswnl commented Mar 23, 2017

jameswnl commented Mar 23, 2017

bdunne Mar 24, 2017

jameswnl Mar 24, 2017

bdunne Mar 24, 2017

jameswnl Mar 24, 2017

bdunne Mar 24, 2017

jameswnl Mar 24, 2017

bdunne Mar 24, 2017

jameswnl Mar 24, 2017

bdunne Mar 24, 2017

jameswnl Mar 24, 2017

mzazrivec commented Mar 27, 2017

jameswnl commented Mar 27, 2017

jameswnl commented Mar 27, 2017

jameswnl commented Mar 27, 2017

bdunne Mar 27, 2017

jameswnl Mar 27, 2017

bdunne Mar 27, 2017

jameswnl Mar 27, 2017

bdunne Mar 27, 2017

jameswnl Mar 27, 2017

miq-bot commented Mar 27, 2017

bdunne Mar 27, 2017

jameswnl Mar 27, 2017

bdunne Mar 27, 2017

jameswnl Mar 27, 2017

martinpovolny commented Apr 4, 2017 •

edited

Loading

simaishi commented Apr 4, 2017

jameswnl commented Apr 4, 2017

simaishi commented Apr 4, 2017

Prepare parameter hash before passing to Tower API credential CU #14483

Prepare parameter hash before passing to Tower API credential CU #14483

Conversation

jameswnl commented Mar 23, 2017

jameswnl commented Mar 23, 2017

miq-bot commented Mar 23, 2017

jameswnl commented Mar 23, 2017

jameswnl commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzazrivec commented Mar 27, 2017

jameswnl commented Mar 27, 2017

jameswnl commented Mar 27, 2017

jameswnl commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpovolny commented Apr 4, 2017 • edited Loading

simaishi commented Apr 4, 2017

jameswnl commented Apr 4, 2017

simaishi commented Apr 4, 2017

martinpovolny commented Apr 4, 2017 •

edited

Loading