ManageIQ / manageiq-automation_engine

Automation engine for ManageIQ
Apache License 2.0
11 stars 74 forks source link

Use Ansible Workflow/Job Template Common Parent Class #557

Closed nasark closed 1 month ago

nasark commented 2 months ago

Previously Ansible AE methods were using the ::ExternalAutomationManager::ConfigurationScript class however this is not a parent class for workflow templates ::ConfigurationWorkflow, and so the AE method would fail on the call https://github.com/ManageIQ/manageiq-automation_engine/blob/master/lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_ansible_template_method.rb#L14 when using workflow templates

@miq-bot assign @agrare @miq-bot add_label bug, radjabov/yes? @miq-bot add_reviewer @agrare

agrare commented 2 months ago

@nasark can you point me to where this is called from? I'd like to try to track the history to see if anything changed because it looks like this would have never worked for AnsibleTower ConfigurationWorkflows.

miq-bot commented 2 months ago

Checked commit https://github.com/nasark/manageiq-automation_engine/commit/0dd243989649bf67bf532c83aec0cb34cb5da285 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :star:

nasark commented 2 months ago

@nasark can you point me to where this is called from? I'd like to try to track the history to see if anything changed because it looks like this would have never worked for AnsibleTower ConfigurationWorkflows.

@agrare https://github.com/ManageIQ/manageiq-automation_engine/blob/master/lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_method.rb#L27 - Should be here but I can't find where the caller method is invoked from

This might be a side effect of the AWX changes a while ago https://github.com/ManageIQ/manageiq/commit/a230e47e763e770089a7d4e40368dececee32e5e#diff-e77a7fcdb2c229a4e365cc4639d52f072e2108bf2a634424156853d39c05f7ec, maybe we missed ::ExternalAutomationManager::ConfigurationScript in the ConfigurationWorkflow class hiearchy?

Fryguy commented 2 months ago

would it be possible to add a spec for this? (or perhaps a spec for the caller of this?)

Fryguy commented 2 months ago

The class hierarchy is

So, this PR is correct, even though technically it would pull in EmbeddedAutomationManager::ConfigurationScript as well. Considering this part of the code is just looking up the record, I think this is ok. Alternatively we could change the query to lookup from each of ExternalAutomationManager::ConfigurationScript and ExternalAutomationManager::ConfigurationWorkflow, but that feels overly complicated.

So, I'm :+1: for this PR.

nasark commented 2 months ago

@agrare @Fryguy Added some specs for running a workflow template AE method

agrare commented 2 months ago

@nasark were you able to track down that other similar failure that occurs later on after this patch was applied?

agrare commented 1 month ago

Failure related to running this fixed in https://github.com/ManageIQ/manageiq-providers-awx/pull/36

Fryguy commented 4 weeks ago

Backported to radjabov in commit 08fdfca67b7887867fa66f192c0f19b1ac4b0c65.

commit 08fdfca67b7887867fa66f192c0f19b1ac4b0c65
Author: Adam Grare <adam@grare.com>
Date:   Thu Sep 19 12:59:22 2024 -0400

    Merge pull request #557 from nasark/workflow_template_support

    Use Ansible Workflow/Job Template Common Parent Class

    (cherry picked from commit 4e555e69b709077e4600ff35a5a7ff0d2adc1598)