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

Add destroy units #812

Merged
merged 2 commits into from Mar 15, 2023
Merged

Add destroy units #812

merged 2 commits into from Mar 15, 2023

Conversation

cderici
Copy link
Member

@cderici cderici commented Mar 8, 2023

Description

This brings back the separate model.destroy_units(*units) as a quality of life feature.

Fixes #811

QA Steps

All the regular tests should pass. Additionally, this also adds a separate integration test for destroy_units, so the following should be passing (I tried it on juju 3.1).

tox -e integration -- tests/integration/test_model.py::test_destroy_units

Notes & Discussion

As a future reference, this was changed in #791.

Comment on lines +2022 to +2023
for u in unit_names:
await self.destroy_unit(u, destroy_storage, dry_run, force, max_wait)
Copy link
Contributor

@addyess addyess Mar 8, 2023

Choose a reason for hiding this comment

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

you can do this or this:

Suggested change
for u in unit_names:
await self.destroy_unit(u, destroy_storage, dry_run, force, max_wait)
await jasyncio.gather(*(
self.destroy_unit(u, destroy_storage, dry_run, force, max_wait)
for u in unit_names
))

While I believe your proposed solution here would get the job done, it may be quicker to not issue the Destroy sequentially. I don't know how long it actually takes to actually start one destroy_unit

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless it makes things significantly slower, I'd prefer to depend on asyncio as little as possible as we ultimately want to get rid of it. I didn't observe such performance deficit when I was running the tests, but feel free to run a comparison on it if you're curious 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

nope it's totally your call here! thanks for the PR. LGTM

@juanmanuel-tirado
Copy link
Contributor

Test unit annotations is failing
[gw0] [ 85%] FAILED tests/integration/test_model.py::test_unit_annotations

Copy link
Contributor

@juanmanuel-tirado juanmanuel-tirado left a comment

Choose a reason for hiding this comment

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

LG2M

@cderici
Copy link
Member Author

cderici commented Mar 15, 2023

That failing test FAILED tests/integration/test_controller.py::test_secrets_backend_lifecycle (failing with ConnectionRefusedError: [Errno 111] Connection refused) is a known problem that's unrelated to this PR. @juanmanuel-tirado We should address that separately in another one.

@cderici
Copy link
Member Author

cderici commented Mar 15, 2023

/merge

@jujubot jujubot merged commit 5c4910b into juju:master Mar 15, 2023
6 of 8 checks passed
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.

[Bug]: model.destroy_units signature changed breaking compatability
4 participants