Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pluggable credential types #22995

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Pluggable credential types #22995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions app/models/authentication.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,6 @@ def self.new(*args, &block)

RETRYABLE_STATUS = %w[error unreachable].freeze

CREDENTIAL_TYPES = {
:external_credential_types => 'ManageIQ::Providers::ExternalAutomationManager::Authentication',
:embedded_ansible_credential_types => 'ManageIQ::Providers::EmbeddedAnsible::AutomationManager::Credential',
:workflows_credential_types => 'ManageIQ::Providers::Workflows::AutomationManager::Credential'
}.freeze

# FIXME: To address problem with url resolution when displayed as a quadicon,
# but it's not *really* the db_name. Might be more proper to override `to_partial_path`
def self.db_name
Expand Down Expand Up @@ -142,10 +136,14 @@ def assign_values(options)
self.attributes = options
end

def self.build_credential_options
CREDENTIAL_TYPES.each_with_object({}) do |(k, v), hash|
hash[k] = v.constantize.descendants.each_with_object({}) do |klass, fields|
fields[klass.name] = klass::API_OPTIONS if defined? klass::API_OPTIONS
def self.credential_types
credential_subclasses = descendants.select { |d| d.try(:credential_type) }.sort_by(&:name)
agrare marked this conversation as resolved.
Show resolved Hide resolved

credential_subclasses.each_with_object({}) do |klass, credential_hash|
credential_hash[klass.credential_type.to_sym] ||= {}

if defined?(klass::API_OPTIONS)
credential_hash[klass.credential_type.to_sym].merge!({klass.name => klass::API_OPTIONS})
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
class ManageIQ::Providers::EmbeddedAnsible::AutomationManager::Credential < ManageIQ::Providers::EmbeddedAutomationManager::Authentication
FRIENDLY_NAME = "Embedded Ansible Credential".freeze

def self.credential_type
"embedded_ansible_credential_types"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we even need this method. It acts as a key into a hash that's presented over the API, but
a) maybe we don't need the key at all
b) if we do need some sort of key, just use the class name
c) if we do need the key and it can't be the class name, then just derive the key with something like "#{klass.name.gsub("::", "_").underscore}_credential_types

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I'm fine merging as is, because it keeps the interface identical to how it was previously, but makes it pluggable, which is progress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think https://github.com/ManageIQ/manageiq/pull/22995/files#r1567779476 answers this, we need it for now because of how the UI splits external / internal automation controllers up ignoring the actual EMS object and assuming there is only one of the internal types.

end

Comment on lines +4 to +7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE I think once we get away from separate "embedded_workflows" and "embedded_ansible" controllers we can drop these "groups" of credential types, because you would just call OPTIONS and pass in a type like we do with ext_management_systems. So you would as for the credential types for the Workflows AutomationManager or the EmbeddedAnsible AutomationManager.

But for now this keeps compatibility without having to hard code all of the types 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't thin OPTIONS supports the collection_class, but it could.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy we pass in type=class_name to OPTIONS, that is how we get DDF params for a particular EMS once you've selected it in the dropdown

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh interesting - we probably should be passing in collection_class. The type filtering doesn't work with the "middle" classes, whereas collection_class does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, clarification - type is fine for the purpose listed which is an EMS is selected and then we filter on that EMS. collection_class would be for other purposes.

private_class_method def self.queue_role
"embedded_ansible"
end
Expand Down
3 changes: 3 additions & 0 deletions app/models/manageiq/providers/external_automation_manager/authentication.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
class ManageIQ::Providers::ExternalAutomationManager::Authentication <
ManageIQ::Providers::AutomationManager::Authentication
def self.credential_type
"external_credential_types"
end
end