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

[WIP] Fixes #36341 - Refactor ansible runner artifacts processing #84

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nofaralfasi
Copy link
Contributor

No description provided.

@nofaralfasi
Copy link
Contributor Author

Hello @adamruzicka,
I would like to proceed with the refactoring process you suggested here #78 (review). However, I've encountered some challenges along the way. As you're aware, the AnsibleRunner class inherits from ::Proxy::Dynflow::Runner::Parent, leading to some methods and variables being exclusively accessible within the AnsibleRunner context. Notable examples include methods like publish_exit_status and broadcast_data, as well as variables such as @outputs, @exit_statuses, and @targets.

My struggle lies in finding an elegant and well-organized approach to extract the code responsible for artifacts processing into a separate class. This separation is crucial for enabling focused testing of the artifacts processing without reliance on the AnsibleRunner components. Although I can relocate certain parts of the artifacts processing code to a new class, there remains the need to directly test the AnsibleRunner.

I opened this PR to provide a preview of my progress thus far, even though it's still in progress.
I'm open to any insights or suggestions you might have regarding this challenge. Thanks!

@adamruzicka
Copy link
Contributor

First, in this case I'd probably embrace the object oriented nature of ruby, the class instance variable in ArtifactsProcessor.artifacts_reader could backfire. In AnsibleRunner#initialize I'd just create a new instance of the ArtifactsProcessor and then work with that instance for the rest of the run.

It depends where you want to draw the line. I would probably try to separate the concerns as much as possible, making AnsibleRunner only deal with the outer interface and offload as much as we can to the processor, the processor should give back data that doesn't need any further processing. There are 3 or so things (output for host, output for all hosts, host exited with code) that can get communicated to the outside. It would be nice if the processor could look at raw ansible events and give AnsibleRunner a list of processed and simplified events which AnsibleRunner could then just communicate out.

This should also address the issue with certain methods not being accessible in certain places. Just pass the data where it needs to be and then create an interface between the two parts.

This commit enhances the AnsibleRunner to improve functionality,
and introduces the ArtifactsProcessor for better artifact handling.
@nofaralfasi nofaralfasi force-pushed the refactore_ansible_runner branch from d5df5a1 to d9102e9 Compare September 27, 2023 10:28
@nofaralfasi
Copy link
Contributor Author

This should also address the issue with certain methods not being accessible in certain places. Just pass the data where it needs to be and then create an interface between the two parts.

What do you mean by that? Should we also pass arguments like @outputs and @exit_statuses?

I updated the code to align more closely with our previous discussion, taking into account the feedback from your earlier comment. I would greatly appreciate any further suggestions or feedback you might have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants