-
Notifications
You must be signed in to change notification settings - Fork 897
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.
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
Integrate with external Tower Workflow #17440
Conversation
@Fryguy can you take a look? Some modeling work here and so would like to gather feedback early on. thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks great
ff91490
to
b49f6c8
Compare
|
||
has_many :configured_systems, :dependent => :destroy, :foreign_key => "manager_id" | ||
has_many :configuration_profiles, :dependent => :destroy, :foreign_key => "manager_id" | ||
has_many :configuration_scripts, :dependent => :destroy, :foreign_key => "manager_id" | ||
has_many :workflows, :dependent => :destroy, :foreign_key => "manager_id" | ||
has_many :workflow_nodes, :dependent => :destroy, :foreign_key => "manager_id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this comment in the associated schema change, but I think these should be prefixed with configuration
to make sure we know why the underlying tables exist.
I'm worried that just calling them workflow
makes it look like we're introducing yet another state machine implementation. When in reality, this is inventory data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
def has_workflows(options = {}) | ||
has_inventory({ | ||
:model_class => ::Workflow, | ||
:manager_ref => [:manager_ref], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a good idea of what's going to be used for the manager_ref
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the native id in Tower for this workflow. We need this in order to ask Tower to execute a particular workflow.
befe6e2
to
cd60675
Compare
@@ -7,10 +7,12 @@ class ManageIQ::Providers::AutomationManager < ManageIQ::Providers::BaseManager | |||
require_nested :InventoryGroup | |||
require_nested :InventoryRootGroup | |||
require_nested :OrchestrationStack | |||
require_nested :ConfigurationWorkflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sort alphabetically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -5,4 +5,5 @@ class ManageIQ::Providers::ExternalAutomationManager < ManageIQ::Providers::Auto | |||
require_nested :ConfigurationScriptSource | |||
require_nested :ConfiguredSystem | |||
require_nested :OrchestrationStack | |||
require_nested :ConfigurationWorkflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -3,6 +3,16 @@ module ManagerRefresh::Inventory::AutomationManager | |||
include ::ManagerRefresh::Inventory::Core | |||
|
|||
class_methods do | |||
def has_automation_manager_configuration_workflows(options = {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be renamed as Rubocop suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are others, so I am not sure if we want to do this. Probably not this PR, even if we want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Ladas thinking about the naming about these has_*
, are there meant to be a predicate? or they should be add_*
?
That could be confusing rubocop...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's not a question (rubocop is wrong), so let's keep this consistent. FYI @slemrmartin is redoing the interface, so it will not look like this at all
1525b95
to
448084e
Compare
Checked commit jameswnl@dd1215d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/manager_refresh/inventory/automation_manager.rb
app/models/manager_refresh/inventory/core.rb
|
Based on ManageIQ/manageiq#17440 (transferred from ManageIQ/manageiq@11b6c65)
Based on ManageIQ/manageiq#17440 (transferred from ManageIQ/manageiq@11b6c65)
https://bugzilla.redhat.com/show_bug.cgi?id=1468339
Part of #17441