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 juju-run to invoke juju-reboot for unit reboot #575

Merged
merged 2 commits into from
Sep 20, 2022
Merged

Use juju-run to invoke juju-reboot for unit reboot #575

merged 2 commits into from
Sep 20, 2022

Conversation

dshcherb
Copy link
Collaborator

Issue #921 has the context for the change: in order to avoid triggering
reboots asynchronously to juju hook executions (when juju ssh reboot
is done).

openstack-charmers/zaza-openstack-tests#921

Issue #921 has the context for the change: in order to avoid triggering
reboots asynchronously to juju hook executions (when `juju ssh reboot`
is done).

openstack-charmers/zaza-openstack-tests#921
generic_utils.juju_reboot(_unit)
self.subprocess.check_call.assert_called_once_with(
['juju', 'ssh', _unit,
f'sudo juju-run -u {_unit} "juju-reboot --now"'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tend not to use f-strings in zaza purely due to the python3.5 support. We need to formally make a break on it, but zaza still tentatively supports py35!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7417002

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test code still uses the f-string? I see the code-under-test does use .format().

@ajkavanagh
Copy link
Collaborator

Now all the functional tests are failing, which is unrelated to the changes.

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Base: 89.22% // Head: 89.19% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (7417002) compared to base (8f9f9c7).
Patch coverage: 62.50% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #575      +/-   ##
==========================================
- Coverage   89.22%   89.19%   -0.03%     
==========================================
  Files          44       44              
  Lines        4594     4610      +16     
==========================================
+ Hits         4099     4112      +13     
- Misses        495      498       +3     
Impacted Files Coverage Δ
zaza/utilities/generic.py 82.06% <57.14%> (-0.69%) ⬇️
zaza/utilities/machine_os.py 100.00% <100.00%> (ø)
zaza/__init__.py 88.59% <0.00%> (+0.97%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dshcherb
Copy link
Collaborator Author

I gave it a try here https://review.opendev.org/c/x/charm-ovn-dedicated-chassis/+/857878/2#message-37afbdf3f745d77719aa43afe5234aaa87d70468 but got an unrelated failure which actually has to do with a regression I introduced to charm-layer-ovn (openstack-charmers/charm-layer-ovn#71 - a fix).

When a fix is landed I'll give it another go.

@ajkavanagh
Copy link
Collaborator

I gave it a try here https://review.opendev.org/c/x/charm-ovn-dedicated-chassis/+/857878/2#message-37afbdf3f745d77719aa43afe5234aaa87d70468 but got an unrelated failure which actually has to do with a regression I introduced to charm-layer-ovn (openstack-charmers/charm-layer-ovn#71 - a fix).

When a fix is landed I'll give it another go.

Okay, sounds good.

@fnordahl
Copy link
Collaborator

Raised https://review.opendev.org/c/x/charm-ovn-chassis/+/858491 to help test this change instead of the in-flight review on the ovn-dedicated-chassis charm.

Copy link
Collaborator

@fnordahl fnordahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fnordahl fnordahl dismissed ajkavanagh’s stale review September 20, 2022 13:16

To me it appears the author has addressed the review comments falling back to .format in the runtime code, and I know you're busy with other stuff. So in the interest of foreward movement I'll push forward. You're right in the unit test containing a f-string, and the gate check for zaza itself is Python 3.6+.

@fnordahl fnordahl merged commit f39d039 into openstack-charmers:master Sep 20, 2022
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.

3 participants