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.

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

Use Ansible Workflow/Job Template Common Parent Class #557

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

nasark
Copy link
Member

@nasark nasark commented Aug 22, 2024

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
Copy link
Member

agrare commented Aug 22, 2024

@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
Copy link
Member

miq-bot commented Aug 22, 2024

Checked commit nasark@0dd2439 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. ⭐

@nasark
Copy link
Member Author

nasark commented Aug 22, 2024

@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 ManageIQ/manageiq@a230e47#diff-e77a7fcdb2c229a4e365cc4639d52f072e2108bf2a634424156853d39c05f7ec, maybe we missed ::ExternalAutomationManager::ConfigurationScript in the ConfigurationWorkflow class hiearchy?

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2024

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

@Fryguy
Copy link
Member

Fryguy commented Sep 3, 2024

The class hierarchy is

  • AutomationManager::ConfigurationScript
    • ExternalAutomationManager::ConfigurationScript
      • Awx::AutomationManager::ConfigurationScript
        • AnsibleTower::AutomationManager::ConfigurationScript
    • EmbeddedAutomationManager::ConfigurationScript
      • Workflows::AutomationManager::WorkflowInstance
        • EmbeddedAnsible::AutomationManager::ConfigurationWorkflow
    • AutomationManager::ConfigurationWorkflow
      • ExternalAutomationManager::ConfigurationWorkflow
        • Awx::AutomationManager::ConfigurationWorkflow
          • AnsibleTower::AutomationManager::ConfigurationWorkflow

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 👍 for this PR.

@nasark
Copy link
Member Author

nasark commented Sep 6, 2024

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

@agrare
Copy link
Member

agrare commented Sep 6, 2024

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

@agrare
Copy link
Member

agrare commented Sep 19, 2024

Failure related to running this fixed in ManageIQ/manageiq-providers-awx#36

@agrare agrare merged commit 4e555e6 into ManageIQ:master Sep 19, 2024
3 checks passed
@Fryguy
Copy link
Member

Fryguy commented Oct 8, 2024

Backported to radjabov in commit 08fdfca.

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)

Fryguy pushed a commit that referenced this pull request Oct 8, 2024
Use Ansible Workflow/Job Template Common Parent Class

(cherry picked from commit 4e555e6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants