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

Remove virt.pool_delete fast parameter #54475

Merged
merged 3 commits into from Dec 19, 2019
Merged

Conversation

cbosdo
Copy link
Collaborator

@cbosdo cbosdo commented Sep 13, 2019

What does this PR do?

There are two reasons to remove this parameter without deprecating it:

  • the meaning has been mistakenly inversed
  • fast=True will raise an exception for every libvirt storage backend
    since that flag has never been implemented in any of those.

What issues does this PR fix or reference?

Fixes issue #54474

Previous Behavior

salt 'kvmsrv' virt.pool_delete mypool fails with a libvirt exception

New Behavior

salt 'kvmsrv' virt.pool_delete mypool actually deletes the pool is that function is supported by the libvirt storage backend.

Tests written?

No, trivial function even when the fast parameter existed, even simpler now.

Commits signed with GPG?

Yes

@xeacott
Copy link
Contributor

@xeacott xeacott commented Oct 14, 2019

I took a look at this PR and the area of code it touches, and I am certainly no expect but I see that you @cbosdo have quite a lot of code in this file so I will approve it. However, I will ask that you rebase and target master with your PR. This is quite a recent change with branching strategy and we're expecting it to make releasing in the future much easier and less problematic 😄

@xeacott xeacott self-requested a review Oct 14, 2019
@cbosdo cbosdo requested a review from as a code owner Oct 24, 2019
@ghost ghost requested a review from cmcmarrow Oct 24, 2019
@cbosdo cbosdo changed the base branch from 2019.2.1 to master Oct 24, 2019
@cbosdo
Copy link
Collaborator Author

@cbosdo cbosdo commented Oct 24, 2019

@xeacott rebased on master and added a unit tests for that function... even though it doesn't test that parameter removal

@xeacott
Copy link
Contributor

@xeacott xeacott commented Oct 31, 2019

Thank you!

@cbosdo cbosdo force-pushed the virt_pool_delete_fix branch from a0b9d54 to e981ac1 Compare Nov 5, 2019
@cbosdo cbosdo mentioned this pull request Nov 5, 2019
There are two reasons to remove this parameter without deprecating it:
 * the meaning has been mistakenly inversed
 * fast=True will raise an exception for every libvirt storage backend
since that flag has never been implemented in any of those.

Fixes issue saltstack#54474
@cbosdo cbosdo force-pushed the virt_pool_delete_fix branch from e981ac1 to f530047 Compare Dec 6, 2019
@dwoz
Copy link
Contributor

@dwoz dwoz commented Dec 19, 2019

The windows tests are passing but the build failed due to some left open file handles.

dwoz
dwoz approved these changes Dec 19, 2019
@dwoz dwoz merged commit 0280328 into saltstack:master Dec 19, 2019
48 of 49 checks passed
@cbosdo cbosdo deleted the virt_pool_delete_fix branch Dec 20, 2019
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.

None yet

3 participants