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 buggy start parameter from virt.pool_running docstring #57450

Merged
merged 6 commits into from
Aug 24, 2020

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented May 25, 2020

What does this PR do?

As mentioned by issue #57275, the start parameter in virt.pool_running
documentation is not implemented at all. Remove it from the doc.

What issues does this PR fix or reference?

Fixes: #57275

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@cbosdo cbosdo requested a review from a team as a code owner May 25, 2020 07:30
@ghost ghost requested review from Akm0d and removed request for a team May 25, 2020 07:30
Akm0d
Akm0d previously approved these changes May 25, 2020
@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 10, 2020

arg, rebased on master and review lost!

@sagetherage sagetherage added Magnesium Mg release after Na prior to Al Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around labels Jul 22, 2020
@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 23, 2020

Rebased on latest master

As mentioned by issue saltstack#57275, the start parameter in virt.pool_running
documentation is not implemented at all. Remove it from the doc.
@cbosdo
Copy link
Contributor Author

cbosdo commented Jul 31, 2020

rebased on master. @dwoz Since this is a doc-only fix, could this be merged?

@ScriptAutomate
Copy link
Contributor

Just want to confirm that this an error in the documentation and not an error in the functionality of the state:

The referenced execution module salt.modules.virt.pool_define by states.virt.pool_defined does have a start param, which is defaulted to True:

But, the state where you are modifying the documentation is defaulting to using start=False (states.virt.pool_running which calls states.virt.pool_defined has this within it) when calling the execution module:

Was this param documented because the state function for virt.pool_defined is meant to have a start param with a default value?

If this is not the case, and it is just a documented parameter that doesn't exist (and wasn't meant to exist) for states.virt.pool_running, then I'll approve this change.

@cbosdo
Copy link
Contributor Author

cbosdo commented Aug 24, 2020

Was this param documented because the state function for virt.pool_defined is meant to have a start param with a default value?

If this is not the case, and it is just a documented parameter that doesn't exist (and wasn't meant to exist) for states.virt.pool_running, then I'll approve this change.

The start parameter isn't supposed to exist since there are two separate states for that. The documentation was added simply with a careless copy/paste of mine.

@dwoz dwoz merged commit f0b953b into saltstack:master Aug 24, 2020
@cbosdo cbosdo deleted the virt-doc-fix branch August 25, 2020 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Documentation Relates to Salt documentation Magnesium Mg release after Na prior to Al severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] salt.states.virt.pool_running doesn't accept start as parameter
5 participants