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

feat(wait_task): Add a wait_task module. #1647

Closed
wants to merge 1 commit into from

Conversation

JGodin-C2C
Copy link
Contributor

What :

And a new module that simply expose the wait_for_task function
This allows to wait for a asynchronous task to finish before continuing to apply the playbook.

How

It simply expose the function "wait_for_task" that was already available as a helper.

Related

fixes #1609

tests

Have been tested with a custom/minimal playbook on an internal satellite server.

@JGodin-C2C JGodin-C2C force-pushed the develop branch 4 times, most recently from 6969c11 to ef9df5a Compare July 20, 2023 14:49
@JGodin-C2C
Copy link
Contributor Author

JGodin-C2C commented Jul 20, 2023

@evgeni Failing tests does not seems to be part of my PR.

@evgeni
Copy link
Member

evgeni commented Jul 20, 2023

They are, or at least partially.

Have a look at the 3.10/2.12 sanity failures here: https://github.com/theforeman/foreman-ansible-modules/actions/runs/5612522667/job/15206459221#step:13:1

(The newer ansible versions fail for some reason, but that is certainly unrelated)

I won't have time to properly review this PR before next week, but wanted to ask if you considered naming the module "wait_for_task" which (IMHO) would better fit with other wait_for_… modules that exist in Ansible core.

@JGodin-C2C
Copy link
Contributor Author

A ! i see, the problem is WAY above the actual failure. I will fix.
Also, i can totally rename it "wait_for_task"

@JGodin-C2C JGodin-C2C force-pushed the develop branch 5 times, most recently from 8a69936 to 6b300fe Compare July 21, 2023 13:41
@JGodin-C2C
Copy link
Contributor Author

Seems much better.

CHANGELOG.rst Outdated Show resolved Hide resolved
meta/runtime.yml Outdated Show resolved Hide resolved
plugins/modules/wait_for_task.py Outdated Show resolved Hide resolved
plugins/modules/wait_for_task.py Outdated Show resolved Hide resolved
@JGodin-C2C JGodin-C2C force-pushed the develop branch 2 times, most recently from d13e5ed to 7c35d6c Compare July 29, 2023 08:35
@JGodin-C2C JGodin-C2C force-pushed the develop branch 5 times, most recently from fac451c to 49def46 Compare August 7, 2023 09:07
And a new module that simply expose the wait_for_task function

Signed-off-by: Julien Godin <[email protected]>
@JGodin-C2C
Copy link
Contributor Author

All green ! 💪

@JGodin-C2C
Copy link
Contributor Author

Ok, what would be the next step to see this merged ?

@evgeni
Copy link
Member

evgeni commented Aug 11, 2023

Me finding the time to finalize the review ;-)
Which I did this morning!

The only "real" thing I wanted/had to change were the tests -- I didn't like to have to do a full publish to get them to run and made them delete a product, which is also an async action we can wait on.

While at it, I also extended the examples a bit ;-)

As I couldn't push to your fork, I opened a second PR with your commit as the first one, and then my changes in mostly logical chunks: #1656

Once CI passes on that one I'll merge it (or you can pull the changes into this PR if you prefer to have the original one to be "closed as merged" -- I don't mind either way)

@evgeni
Copy link
Member

evgeni commented Aug 11, 2023

merged as #1656

@evgeni evgeni closed this Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants