-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix unit.run
to support Juju 2.9 and 3.x
#655
Fix unit.run
to support Juju 2.9 and 3.x
#655
Conversation
I will run this change in one of the charms, with the CI. - https://review.opendev.org/c/openstack/charm-mysql-innodb-cluster/+/915994 |
@rgildein , can you take the chance of enabling juju 3.4 in the CI? - https://github.com/openstack-charmers/zaza/blob/master/.github/workflows/tox.yaml#L38 . |
Done |
Add simply helper function with trying to pass parameter block=True to unit.run and catch TypeError and run without parameter block.
a946a2d
to
faf2d52
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #655 +/- ##
==========================================
- Coverage 89.54% 89.53% -0.01%
==========================================
Files 45 45
Lines 4762 4759 -3
==========================================
- Hits 4264 4261 -3
Misses 498 498 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the effort put into this. I'm just waiting for the CI results on this change - https://review.opendev.org/c/openstack/charm-mysql-innodb-cluster/+/915994 - to merge.
note: the ci failure in the 'third' test, it's not related to this change.
|
||
|
||
async def unit_run(unit, command, timeout=None): | ||
"""Help function to help support Juju 2.9 and Juju 3>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could we change this docstring to be more descriptive? For example,
Helper function to support running `unit.run` for both Juju 2.9 and Juju 3.x versions.
unit.run
to support Juju 2.9 and 3.x
jammy https://openstack-ci-reports.ubuntu.com/artifacts/f20/915994/2/check/jammy/f2025fa/ : SUCCESS in 1h 18m 51s the focal job failed, because an undercloud issue, so I won't block this PR on that. |
The run_on_unit require to be awaited for juju 3+, and this fix proposes a solution to check status of action and wait for it if it's "pending". This was original described in issue #649 and marked as fixed by #650, however the action object is never awaitable as it can be seen in my examples below.
I tested in with this script
and here are the results for Juju 2.9
and results for Juju 3.4:
The example of usage on both 2.9 and 3.4 (see logs) can be found here. The func tests are failing, but it's not related with zaza.
fixes: #649