ConfigurationWorkflow to exist only in AutomationManager space #17720

jameswnl · 2018-07-17T17:44:29Z

As a alternative implementation in terms of relationship between automation_manager and configuration_script to #17659

The related change in external tower repo ManageIQ/manageiq-providers-ansible_tower#112
@blomquisg @Fryguy @bzwei @gmcculloug

jameswnl · 2018-07-17T17:55:05Z

app/models/manageiq/providers/automation_manager.rb

@@ -11,7 +11,8 @@ class ManageIQ::Providers::AutomationManager < ManageIQ::Providers::BaseManager

  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 :configuration_scripts,        :dependent => :destroy, :foreign_key => "manager_id",
+                                          :class_name => "::ConfigurationScript"


@Fryguy @blomquisg @gmcculloug @bzwei
This would get back both job_templates and workflows, but I don't like that it's breaching into the base class space.

you don't need the class_name bit, because it's implied.

That is exactly how we'd expected it to work from the original discussion...not sure I understand your concern.

Without the class_name, it will default to the AutomationManager::ConfigurationScript (and that gives only the job_templates)

@Fryguy
Our comments are interwinding and so let me try to clarify again:

We need to pin the class_name to the base class in order to get what we want (get back both job_templates/workflows)

Pinning the relationship (the class_name) from a subclass level object to a base class level object (i.e. ManageIQ::Providers::AutomationManager vs ::ConfigurationScript) is kind of breaching the abstraction layers. It's not the end of the world. But why don't we simply move the relationship to the 2 base classes (EMS vs ConfigurationScript) ?

Fryguy · 2018-07-17T18:16:55Z

app/models/manager_refresh/inventory/core.rb

@@ -43,7 +43,7 @@ def has_configuration_script_payloads(options = {})

    def has_configuration_workflows(options = {})
      has_inventory({
-        :model_class                 => ::ConfigurationWorkflow,
+        :model_class                 => ManageIQ::Providers::AutomationManager::ConfigurationWorkflow,


has_configuration_scripts should have a similar model_class to isolate it to AutomationManager::ConfigurationScript because you want to filter only those on that half of the equation.

Actually...this hardcoding should not be in core...I think I'm missing something fundamental about the refresh modeling to understand this.

@Fryguy But then this would deviate from the relationship defined in AutomationManager (which includes both job_templates and workflows). Do we want that?

(Either way, I am actually have issue with here pointing to a specific subclass in this core location. But I don't have a better solution since we eliminate ::ConfigurationWorkflow)

haha yeah, you comment is exactly my concern, hardcoding the specific subclass in core....
@Ladas can you help?

@Ladas bump

let us brainstorm with @agrare and @slemrmartin on this

Well for one we got rid of this way of defining inventory collections so this will never get used :)

@agrare hm right, I got lost in what got merged. So the new way seems to already solve this? https://github.com/Ladas/manageiq/blob/dd1215d37906f6fd7ee757cdd5510a019bfa7039/app/models/manager_refresh/inventory/automation_manager.rb#L38 , right @slemrmartin ?

@jameswnl as @agrare wrote has_inventory interface is deprecated and will be removed.
Please use add_collection and ManagerRefresh::InventoryCollection::Builder::AutomationManager instead.

Model class is derived automatically, but, as I described #17574 (comment), autoloading (including calling new) is weird when you doesn't have IC object's class defined for each persister's subclass (..::EmbeddedAutomationManager, ...::EmbeddedAnsible etc.).

You can hardcode :model_class as you defined above in ManageIQ::Providers::AnsibleTower::Inventory::Persister::AutomationManager
And redefine it, if you need, in persister's subclasses

jameswnl · 2018-07-20T18:39:28Z

app/models/configuration_script.rb

@@ -2,4 +2,5 @@ class ConfigurationScript < ConfigurationScriptBase
  def self.base_model
    ConfigurationScript
  end
+  belongs_to :manager, :class_name => "ManageIQ::Providers::AutomationManager", :inverse_of => :configuration_scripts


@Fryguy @blomquisg @gmcculloug I have to changed here :class_name to ManageIQ::Providers::AutomationManager to fix spec failure here https://travis-ci.org/ManageIQ/manageiq/jobs/405012910#L870

Ladas · 2018-07-24T08:33:33Z

app/models/manageiq/providers/automation_manager.rb

+           :foreign_key => "manager_id",
+           :class_name  => "::ConfigurationScript",
+           :inverse_of  => :manager
+
  has_many :configuration_workflows,      :dependent => :destroy, :foreign_key => "manager_id", :inverse_of => :manager


won't this fails, when ConfigurationWorkflow was deleted, I think this will need :class_name => "::ConfigurationScript",

It infers to default AutomationManager::ConfigurationWorkflow.

We may have to remove this because now it's "unfair" that we have a query to get back workflows (aka AutomationManager::ConfigurationWorkflow) but no job_templates(aka AutomationManager::ConfigurationScript)

Ladas · 2018-07-24T08:33:57Z

app/models/manageiq/providers/automation_manager.rb

+  has_many :configuration_scripts,
+           :dependent   => :destroy,
+           :foreign_key => "manager_id",
+           :class_name  => "::ConfigurationScript",


this looks redundant, the class should be inferred automatically

The inferred class would be AutomationManager::ConfigurationScript. The requirement here is to return both AutomationManager::ConfigurationScript and AutomationManager::ConfigurationWorkflow

miq-bot · 2018-07-24T12:47:36Z

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

…mation_manager

bzwei · 2018-07-26T20:45:30Z

app/models/manageiq/providers/embedded_automation_manager/configuration_workflow.rb

@@ -1,2 +1,2 @@
-class ManageIQ::Providers::EmbeddedAutomationManager::ConfigurationWorkflow < ManageIQ::Providers::AutomationManager::ConfigurationWorkflow
+class ManageIQ::Providers::EmbeddedAutomationManager::ConfigurationWorkflow < ManageIQ::Providers::AutomationManager::ConfigurationScript


@jameswnl please remove all ConfigurationWorkflow classes from EmbeddedAutomationManager and EmbeddedAnsibleManger. We currently don't support them.

yes, in progress. removing this causing the spec to fail. Because embedded/external share the same set of specs and vcr cassettes.
The vcr part is troublesome because of the allow_unused_http_interactions => false and embedded spec won't retrieve the workflows causing unused HTTP error. Any suggestion?

Shall we keep this in place? As far as UI/Automate are not using them, no material effects (For embedded one, user cannot create a workflow anyway).

Otherwise, we'll have to split the specs and 95% of the specs would have to be duplicated code.

@Fryguy @blomquisg ?

I'm fine with keeping it since it won't be used anyway.

miq-bot · 2018-07-26T21:21:53Z

Checked commits jameswnl/manageiq@50a7589~...0262a2d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 👍

jameswnl · 2018-07-30T16:02:48Z

@miq-bot add_labels providers/ansible_tower, refactor

jameswnl · 2018-07-30T16:03:00Z

@miq-bot remove_label wip

miq-bot · 2018-07-30T16:03:05Z

@jameswnl Cannot apply the following label because they are not recognized: refactor

Fryguy · 2018-07-30T16:23:15Z

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

@@ -26,6 +26,8 @@ class ManageIQ::Providers::EmbeddedAnsible::AutomationManager < ManageIQ::Provid
  require_nested :Refresher
  require_nested :RefreshWorker

+  has_many :configuration_workflows, :dependent => :destroy, :foreign_key => "manager_id", :inverse_of => :manager


Is this needed?

Without this, a bunch specs will fail due to the Inventory collection logic. I can work on a separate PR just to remove this

Right now, the persistor and parser are shared between Embedded and Tower. So not doing this, we will need to add a bunch of conditions to differentiate the 2, or partially separate them. So adding this is actually nicer :-)

So I am fine with Embedded responding to .configuration_workflows, but never refreshing it. (performance wise, this will be recognized as noop in embedded refresh and skipped fast)

Fryguy · 2018-07-31T18:19:33Z

Merged, but I have a concern about refresh, even with the configuration_workflows association. We need to verify that we don't create and delete configuration_scripts over and over on refresh. Easiest way is to verify the ids are the same before and after.

Reverted some of the changes that were made to use ManageIQ/manageiq#17580 and adjusted code to use subclass in ManageIQ/manageiq#17720 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1590441 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1468339

miq-bot added the wip label Jul 17, 2018

jameswnl force-pushed the workflow3 branch from b6b4bd3 to 21d4f35 Compare July 17, 2018 17:47

jameswnl commented Jul 17, 2018

View reviewed changes

jameswnl mentioned this pull request Jul 17, 2018

ConfigurationWorkflow to exist only in AutomationManager space #17659

Closed

Fryguy reviewed Jul 17, 2018

View reviewed changes

jameswnl force-pushed the workflow3 branch 2 times, most recently from ce57cf5 to a354946 Compare July 20, 2018 18:37

jameswnl commented Jul 20, 2018

View reviewed changes

jameswnl force-pushed the workflow3 branch 3 times, most recently from 6ece83f to 5c0831f Compare July 23, 2018 20:47

Ladas reviewed Jul 24, 2018

View reviewed changes

miq-bot added the unmergeable label Jul 24, 2018

jameswnl force-pushed the workflow3 branch from 5c0831f to b11fb56 Compare July 24, 2018 14:22

miq-bot removed the unmergeable label Jul 24, 2018

ConfigurationWorkflow to exist only in AutomationManager space

50a7589

jameswnl force-pushed the workflow3 branch 3 times, most recently from d90d02b to 3ece6a5 Compare July 26, 2018 19:29

remove configuration_workflow from app/models/manageiq/providers/auto…

0262a2d

…mation_manager

bzwei reviewed Jul 26, 2018

View reviewed changes

jameswnl force-pushed the workflow3 branch from 3ece6a5 to 0262a2d Compare July 26, 2018 21:20

jameswnl mentioned this pull request Jul 26, 2018

ConfigurationWorkflow only exists in under AnsibleTower::AutomationMa… ManageIQ/manageiq-providers-ansible_tower#112

Merged

jameswnl changed the title ~~[WIP] Alt: ConfigurationWorkflow to exist only in AutomationManager space~~ ConfigurationWorkflow to exist only in AutomationManager space Jul 30, 2018

miq-bot added providers/ansible_tower and removed wip labels Jul 30, 2018

Fryguy reviewed Jul 30, 2018

View reviewed changes

h-kataria mentioned this pull request Jul 30, 2018

Show Configuration Workflows in UI ManageIQ/manageiq-ui-classic#4138

Merged

Fryguy merged commit 0afd317 into ManageIQ:master Jul 31, 2018

jameswnl deleted the workflow3 branch July 31, 2018 18:31

jameswnl mentioned this pull request Jul 31, 2018

remove 'require_nested :ConfigurationWorkflow' from parent space #17782

Merged

Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Jul 31, 2018

Fryguy self-assigned this Jul 31, 2018

Fryguy added the enhancement label Jul 31, 2018

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConfigurationWorkflow to exist only in AutomationManager space #17720

ConfigurationWorkflow to exist only in AutomationManager space #17720

jameswnl commented Jul 17, 2018 •

edited

jameswnl Jul 17, 2018

Fryguy Jul 17, 2018

Fryguy Jul 17, 2018

jameswnl Jul 17, 2018

jameswnl Jul 17, 2018 •

edited

Fryguy Jul 17, 2018

Fryguy Jul 17, 2018

jameswnl Jul 17, 2018

jameswnl Jul 17, 2018

gmcculloug Jul 23, 2018

Ladas Jul 23, 2018

agrare Jul 23, 2018

Ladas Jul 24, 2018

slemrmartin Jul 24, 2018 •

edited

jameswnl Jul 20, 2018 •

edited

Ladas Jul 24, 2018

jameswnl Jul 24, 2018

Ladas Jul 24, 2018

jameswnl Jul 24, 2018

miq-bot commented Jul 24, 2018

bzwei Jul 26, 2018

jameswnl Jul 26, 2018

jameswnl Jul 26, 2018

Fryguy Jul 26, 2018

miq-bot commented Jul 26, 2018

jameswnl commented Jul 30, 2018

jameswnl commented Jul 30, 2018

miq-bot commented Jul 30, 2018

Fryguy Jul 30, 2018

jameswnl Jul 30, 2018

Ladas Jul 31, 2018

Fryguy commented Jul 31, 2018

		@@ -1,2 +1,2 @@
		class ManageIQ::Providers::EmbeddedAutomationManager::ConfigurationWorkflow < ManageIQ::Providers::AutomationManager::ConfigurationWorkflow
		class ManageIQ::Providers::EmbeddedAutomationManager::ConfigurationWorkflow < ManageIQ::Providers::AutomationManager::ConfigurationScript

ConfigurationWorkflow to exist only in AutomationManager space #17720

ConfigurationWorkflow to exist only in AutomationManager space #17720

Conversation

jameswnl commented Jul 17, 2018 • edited

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 Jul 17, 2018 • edited

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

slemrmartin Jul 24, 2018 • edited

Choose a reason for hiding this comment

jameswnl Jul 20, 2018 • edited

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 Jul 24, 2018

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 Jul 26, 2018

jameswnl commented Jul 30, 2018

jameswnl commented Jul 30, 2018

miq-bot commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jul 31, 2018

jameswnl commented Jul 17, 2018 •

edited

jameswnl Jul 17, 2018 •

edited

slemrmartin Jul 24, 2018 •

edited

jameswnl Jul 20, 2018 •

edited