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

virt._get_domain: don't raise an exception if there is no VM #56392

Merged
merged 3 commits into from
Apr 11, 2020

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented Mar 17, 2020

What does this PR do?

Raising an exception if there is no VM in virt._get_domain makes sense if
looking for some VMs, but not when listing all VMs.

What issues does this PR fix or reference?

Previous Behavior

Running this on a hypervisor minion without any VM raises an exception:

salt 'kvm' virt.list_domains

New Behavior

Running the same command without any VM only returns an empty list

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

meaksh
meaksh previously approved these changes Mar 17, 2020
brejoc
brejoc previously approved these changes Mar 17, 2020
Copy link
Contributor

@brejoc brejoc left a comment

Choose a reason for hiding this comment

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

👍

@brejoc
Copy link
Contributor

brejoc commented Mar 17, 2020

@saltstack/team-core Would be cool if we might get a go nor no-go here from you. Then we could still squeeze this into the next SLE release. Thanks and sorry for pushing!

@cbosdo
Copy link
Contributor Author

cbosdo commented Mar 23, 2020

just rebased on top of master

@cbosdo
Copy link
Contributor Author

cbosdo commented Apr 2, 2020

rebased on master

DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 2, 2020
@DmitryKuzmenko
Copy link
Contributor

Thank you for PR! It's perfect. Lets wait for tests.

@DmitryKuzmenko
Copy link
Contributor

@cbosdo could you please resolve merge conflicts?

@cbosdo
Copy link
Contributor Author

cbosdo commented Apr 7, 2020

@DmitryKuzmenko rebased

@cbosdo cbosdo force-pushed the no_vm_fix branch 2 times, most recently from c47ba77 to da7a380 Compare April 7, 2020 09:07
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 7, 2020
Raising an exception if there is no VM in _get_domain makes sense if
looking for some VMs, but not when listing all VMs.
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 9, 2020
Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

Choose a reason for hiding this comment

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

I've slightly updated the test (isort and "/' in check string). Now it should be ready. Lets wait for tests.

@cbosdo
Copy link
Contributor Author

cbosdo commented Apr 10, 2020

@DmitryKuzmenko I've seen that isort and black are sometimes doing conflicting changes... which to believe then?

@DmitryKuzmenko
Copy link
Contributor

@cbosdo this is a good point but I haven't seen such behavior. Do you have an example?

@dwoz dwoz merged commit 9458e19 into saltstack:master Apr 11, 2020
@cbosdo cbosdo deleted the no_vm_fix branch April 11, 2020 06:45
@cbosdo
Copy link
Contributor Author

cbosdo commented Apr 14, 2020

@cbosdo this is a good point but I haven't seen such behavior. Do you have an example?

As an example in PR #55814, running isort salt/states/virt.py will create this diff:

--- a/salt/states/virt.py
+++ b/salt/states/virt.py
@@ -25,7 +25,6 @@ import salt.utils.files
 import salt.utils.stringutils
 import salt.utils.versions
 from salt.exceptions import CommandExecutionError
-
 # Import 3rd-party libs
 from salt.ext import six
 

Applying black on that same file again, removes the change.

@DmitryKuzmenko
Copy link
Contributor

@cbosdo Hah! Really works if run isort and black manually but pre-commit hook doesn't do any change on that file.

@cbosdo
Copy link
Contributor Author

cbosdo commented Apr 15, 2020

yeah it depends on the run order of isort and black

@waynew
Copy link
Contributor

waynew commented Apr 16, 2020

Less the run order, and more the version. When isort 5 is released it will contain the black friendly option to put spaces before comments.

@waynew
Copy link
Contributor

waynew commented Apr 16, 2020

If you run pre-commit run isort you shouldn't see this diff

@cbosdo
Copy link
Contributor Author

cbosdo commented Apr 17, 2020

OK, will try for my next commits :)

@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants