Extensible attributes for Ansible Tower Credentials #13826

jameswnl · 2017-02-08T21:59:18Z

No description provided.

jameswnl · 2017-02-08T21:59:37Z

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

Fryguy · 2017-02-09T15:54:51Z

Hardcoded is fine right now. Don't worry about AlignHash in this instance, but do fix the other rubocops.

bdunne

Overall, I think this is headed in the right direction.

bdunne · 2017-02-09T16:01:51Z

app/models/manageiq/providers/ansible_tower/automation_manager/machine_credential.rb

@@ -1,2 +1,85 @@
 class ManageIQ::Providers::AnsibleTower::AutomationManager::MachineCredential < ManageIQ::Providers::AutomationManager::Authentication
+  def self.fields
+    return {


Why return?

bdunne · 2017-02-09T16:45:09Z

db/migrate/20170206210552_add_options_column_to_authentication.rb

@@ -0,0 +1,5 @@
+class AddOptionsColumnToAuthentication < ActiveRecord::Migration[5.0]
+  def change
+    add_column :notifications, :options, :text


You're pointing to the wrong table...

blomquisg · 2017-02-10T15:33:12Z

app/models/manageiq/providers/ansible_tower/automation_manager/machine_credential.rb

+        :type       => 'string',
+        :required   => false,
+        :label      => 'Description',
+        :help_text  => 'Optional description of this credential.',


These strings need I18N. @himdel is that the _N method?

So.. depends.. it's either N_ or _, the first if you want the string to get in the translation catalog but don't want to translate it right away (meaning you have to pass it through _ at some later point), the latter if you want it translated right away.

Right now, help_text is never used anywhere in the UI, so .. no idea which :).

Either way, the same goes for label

jameswnl · 2017-02-14T07:07:32Z

@Fryguy @bdunne @blomquisg I've added attribute accessor to the options field. Please see if it makes sense.

bdunne

I don't agree with all of the dynamically defined stuff.

bdunne · 2017-02-14T20:24:19Z

app/models/manageiq/providers/ansible_tower/automation_manager/amazon_credential.rb

@@ -1,2 +1,5 @@
 class ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential < ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential
+  ATTRIBUTES = [:description, :username, :security_token].freeze


Not crazy about calling it ATTRIBUTES, I don't want it to be confused with #attributes. Maybe EXTRA_ATTRIBUTES?

👍 for EXTRA_ATTRIBUTES

sure, will update this

bdunne · 2017-02-14T20:30:53Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+module ManageIQ::Providers::AnsibleTower::AutomationManager::CredentialMixin
+  extend ActiveSupport::Concern
+
+  FIELDS = {


I don't think this belongs in the mixin. Not all of these fields apply to all credential types.

bdunne · 2017-02-14T20:37:59Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+        options && options[attr] ? options[attr] : FIELDS[attr][:default]
+      end
+
+      define_method(:"#{attr}=") do |value|


Do we even need writer methods for this? Shouldn't all of this be pulled from refresh?

how about UI?

Why would the UI be writing this? Any UI changes should be an update that is pushed to Tower and the refresh should update the columns.

bdunne · 2017-02-14T20:41:58Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+
+  included do
+    self::ATTRIBUTES.each do |attr|
+      define_method(:"#{attr}") do


Does this work?

yes, check the spec

I guess I'm just surprised at the complexity when it could just be define_method(attr) do

On a side note, I'm kind of amazed at this:

irb(main):011:0> attr = :abc => :abc irb(main):012:0> attr.object_id => 16075548 irb(main):013:0> (:"#{attr}").object_id => 16075548

Either way, I'm still against the dynamically defined methods.

bdunne · 2017-02-14T20:44:55Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+
+  included do
+    self::ATTRIBUTES.each do |attr|
+      define_method(:"#{attr}") do


Dynamically defining methods makes it really hard to troubleshoot later. I don't see the methods changing often, can we just define the necessary methods on the appropriate models? Or set up a few mixins that get included?

bdunne · 2017-02-14T20:51:01Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+        self.options[attr] = value
+      end
+
+      define_method(:"#{attr}_meta") do


Do we need these methods?

Welcome suggestions to remove/add these

Where is the caller?

bdunne · 2017-02-14T20:53:43Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+
+  module ClassMethods
+    def fields
+      self::ATTRIBUTES.each_with_object({}) do |field_key, hash|


There is no need to rebuild this on every call. It should probably be in a constant.

bdunne · 2017-02-14T20:56:21Z

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

@@ -0,0 +1,19 @@
+class DummyCredential < ::Authentication


Please don't create class constants in the test environment. They are not limited in scope to this test and have conflicted in the past and troubleshooting is painful. Instead, if needed:

let(:test_class) do class.new(::Authentication) do ATTRIBUTES = [] etc... end end

bdunne · 2017-02-14T21:00:38Z

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

@@ -0,0 +1,25 @@
+describe ManageIQ::Providers::AnsibleTower::AutomationManager::CredentialMixin do
+  let(:credential) { FactoryGirl.create(:authentication) }
+  let(:credential_class) do


This was updated during review. Pasting my original comment since it still applies...

Please don't create class constants in the test environment. They are not limited in scope to this test and have conflicted in the past and troubleshooting is painful. Instead, if needed:

let(:test_class) do class.new(::Authentication) do ATTRIBUTES = [] etc... end end

cool, wish I had known this earlier! Thanks

Fryguy · 2017-02-14T21:08:26Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+      :help_text  => _('The hostname or IP address to use.'),
+      :max_length => 1024,
+      :encrypted  => false,
+      :default    => ''


If every default is '', why have it?

These are all from Tower API returns (except the :encrypted which is added by me). I think they want this to be generic so that if there is fields with different defaults.

I can remove this if we want to.

Fryguy · 2017-02-14T21:08:43Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential_mixin.rb

+
+  FIELDS = {
+    :description => {
+      :type       => 'string',


I feel like this should be a symbol, but not sure.

Dismissing my concerns..

Fryguy · 2017-02-22T17:00:51Z

There was a concern from @jameswnl and @bdunne how to present the "shared fields" to the UI such that they can build them dynamically, but also so that we can store them in the proper columns in the auth record. My thoughts are as follows:

At the model level, I think extra attributes are a completely separate thing from regular columns, so the EXTRA_ATTRIBUTES hash would only have the extra stuff. Separately, perhaps we have a second constant that represents the "regular" columns.

class ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential < ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential
  COMMON_ATTRIBUTES = {
    :userid => {
      #Might be ok to leave out type because it's column based, so just use the column type
      :label => N_("Access Key"),
      :help_text => N_("AWS Access Key for this credentials")
    }
    :password => {
      :type => :password,
      :label => N_("Secret Token"),
      :help_text => N_("AWS Secret Token for this credentials")
    }
  }.freeze

  EXTRA_ATTRIBUTES = {
    :security_token => {
      :type       => :password,
      :label      => N_('Security Token'),
      :help_text  => N_('Security Token for this credential'),
      :max_length => 1024
    }
  }.freeze

  API_ATTRIBUTES = COMMON_ATTRIBUTES.merge(EXTRA_ATTRIBUTES).freeze # This also enforces that COMMON ones should be presented first
end

Then,

on the API side, the OPTIONS verb can present API_ATTRIBUTES.
on the EMS Refresh side, only things in EXTRA_ATTRIBUTES will go in the options column, and everything else will go into the corresponding regular columns.

cc @jntullo @blomquisg

durandom · 2017-02-23T09:36:17Z

app/models/manageiq/providers/ansible_tower/automation_manager/scm_credential.rb

+      :label      => N_('Private key'),
+      :help_text  => N_('RSA or DSA private key to be used instead of password')
+    }
+  }.freeze


Does this not need API_ATTRIBUTES as well?

yes, it does. Done. thanks

blomquisg · 2017-02-23T13:40:50Z

charting test failures ... kicking tests

jntullo · 2017-02-23T14:27:56Z

@Fryguy should we also have display_name merged into API_ATTRIBUTES as you mentioned here? If we decide not to do it here, I can do it in a follow up PR.

miq-bot · 2017-02-23T18:31:32Z

This pull request is not mergeable. Please rebase and repush.

Fryguy · 2017-02-27T19:00:57Z

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

+      :type       => :choice,
+      :label      => N_('Privilege Escalation'),
+      :help_text  => N_('Privilege escalation method'),
+      :choices    => %w(sudo su pbrun pfexec).insert(0, '')


@jameswnl What is the purpose of the insert(0, ''). Is blank a valid choice, or is this for UI purposes? If the latter, then that's a UI concern, and should not be here in the model at all.

The API takes an empty string as choice of not having escalation privilege set.

Per offline chat with Jason, I've update this to an array

Fryguy · 2017-02-27T19:02:07Z

One more comment @jameswnl . The rest looks good to me.

@Fryguy should we also have display_name merged into API_ATTRIBUTES as you mentioned here? If we decide not to do it here, I can do it in a follow up PR.

Yeah, let's add that in the follow up PR that includes the API changes, if we think it belongs there. @jntullo Are you working on the API changes?

jntullo · 2017-02-27T19:25:47Z

@Fryguy yup I am and can add in those changes 👍

miq-bot · 2017-02-27T20:01:28Z

This pull request is not mergeable. Please rebase and repush.

https://www.pivotaltracker.com/story/show/138861739

miq-bot · 2017-02-27T20:07:29Z

Checked commit jameswnl@2bc24d2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
14 files checked, 19 offenses detected

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

❗ - Line 5, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

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

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

❗ - Line 23, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 24, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 25, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 26, Col 7 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 5, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

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

❗ - Line 5, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

jameswnl force-pushed the tower-creds-2 branch from e029707 to b6b9f74 Compare February 8, 2017 21:59

miq-bot added enhancement providers/ansible_tower wip labels Feb 8, 2017

jameswnl force-pushed the tower-creds-2 branch from b6b9f74 to 71aa933 Compare February 8, 2017 22:00

miq-bot added the sql migration label Feb 8, 2017

bdunne requested changes Feb 9, 2017

View reviewed changes

blomquisg reviewed Feb 10, 2017

View reviewed changes

jameswnl force-pushed the tower-creds-2 branch 8 times, most recently from 76acfaf to dcd74ba Compare February 14, 2017 07:06

jameswnl force-pushed the tower-creds-2 branch 4 times, most recently from 37436b7 to 1b83e6d Compare February 14, 2017 19:11

jntullo mentioned this pull request Feb 14, 2017

[WIP] Add additional options to Authentications #13921

Closed

jameswnl force-pushed the tower-creds-2 branch from 1b83e6d to f4ded8d Compare February 14, 2017 20:54

bdunne requested changes Feb 14, 2017

View reviewed changes

jameswnl force-pushed the tower-creds-2 branch from f4ded8d to ffcf068 Compare February 14, 2017 20:58

bdunne reviewed Feb 14, 2017

View reviewed changes

Fryguy reviewed Feb 14, 2017

View reviewed changes

jameswnl force-pushed the tower-creds-2 branch from 178fa68 to f480692 Compare February 23, 2017 00:11

durandom reviewed Feb 23, 2017

View reviewed changes

blomquisg closed this Feb 23, 2017

blomquisg reopened this Feb 23, 2017

jameswnl force-pushed the tower-creds-2 branch from f480692 to bbe10b3 Compare February 23, 2017 13:58

miq-bot added the unmergeable label Feb 23, 2017

jameswnl force-pushed the tower-creds-2 branch 2 times, most recently from 59a4bce to aca09e2 Compare February 23, 2017 20:19

miq-bot removed the unmergeable label Feb 23, 2017

Fryguy reviewed Feb 27, 2017

View reviewed changes

jameswnl force-pushed the tower-creds-2 branch from aca09e2 to 15a8d8c Compare February 27, 2017 19:58

miq-bot added the unmergeable label Feb 27, 2017

Add options to authentications for Ansible credential details

2bc24d2

https://www.pivotaltracker.com/story/show/138861739

jameswnl force-pushed the tower-creds-2 branch from 15a8d8c to 2bc24d2 Compare February 27, 2017 20:02

miq-bot removed the unmergeable label Feb 27, 2017

Fryguy merged commit 40d2c18 into ManageIQ:master Feb 27, 2017

Fryguy added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 27, 2017

jameswnl deleted the tower-creds-2 branch February 27, 2017 21:58

jameswnl mentioned this pull request Feb 28, 2017

Refresh to pick up extra attributes of Ansible Credentials #14106

Merged

enoodle mentioned this pull request Jun 19, 2017

Adding options field to ext_management_system #15398

Merged

Fryguy added the providers/automation label Jan 24, 2020

		@@ -1,2 +1,5 @@
		class ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential < ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential
		ATTRIBUTES = [:description, :username, :security_token].freeze

Extensible attributes for Ansible Tower Credentials #13826

Extensible attributes for Ansible Tower Credentials #13826

Conversation

jameswnl commented Feb 8, 2017

jameswnl commented Feb 8, 2017

Fryguy commented Feb 9, 2017

bdunne left a 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

jameswnl commented Feb 14, 2017

bdunne left a 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

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

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

Fryguy commented Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blomquisg commented Feb 23, 2017

jntullo commented Feb 23, 2017

miq-bot commented Feb 23, 2017

Choose a reason for hiding this comment

jameswnl Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Feb 27, 2017

jntullo commented Feb 27, 2017

miq-bot commented Feb 27, 2017

miq-bot commented Feb 27, 2017

Fryguy commented Feb 22, 2017 •

edited

Loading

jameswnl Feb 27, 2017 •

edited

Loading