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

Return correct result from module.run state #58752

Merged
merged 9 commits into from
Feb 25, 2021

Conversation

amendlik
Copy link
Contributor

@amendlik amendlik commented Oct 17, 2020

What does this PR do?

Correct two bugs in the module.run state where the wrong result is returned.

  1. When run in test mode, module.run should return None, indicating that changes would be made when not run in test mode
  2. When no functions are passed to the state, it should return False, indicating an invocation error.

What issues does this PR fix or reference?

None

Previous Behavior

  1. When run in test mode, module.run returned True, which indicates that no change would be made in in live mode. This is wrong because modules.run always executes the configured functions
  2. When run with no functions passed, module.run returned True. This should return False to indicate an error.

New Behavior

  1. When run in test mode, module.run returns None, which indicates that changes will be made in in live mode.
  2. When run with no functions passed, module.run return False, which indicate an error.

Merge requirements satisfied?

Commits signed with GPG?

Yes

@amendlik amendlik requested a review from a team as a code owner October 17, 2020 14:09
@amendlik amendlik requested review from Ch3LL and removed request for a team October 17, 2020 14:09
@ghost ghost requested a review from krionbsd October 17, 2020 14:10
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

could you also add a changelog entry as documented here: https://docs.saltstack.com/en/master/topics/development/changelog.html

salt/states/module.py Outdated Show resolved Hide resolved
@amendlik amendlik force-pushed the module-run-state branch 2 times, most recently from ed38d17 to 810e0d7 Compare October 29, 2020 16:41
Ch3LL
Ch3LL previously approved these changes Nov 3, 2020
krionbsd
krionbsd previously approved these changes Nov 9, 2020
@Ch3LL
Copy link
Contributor

Ch3LL commented Dec 10, 2020

@amendlik looks like there are some merge conflicts that need to be cleaned up.

amendlik and others added 3 commits December 11, 2020 10:36
module.run contained a bug where it would return True when run with
test=true. According to the documentation at
http://docs.saltstack.com/ref/states/writing.html#return-data
the state should return None in test mode if a change would be made
without test mode. Since module.run is not idempotent, the appropriate
return should always be None, as long as no errors were encountered.
@amendlik
Copy link
Contributor Author

@Ch3LL Ok, I rewrote my changes so they will merge with the updated files. What else do I need to do to get this merged?

@amendlik
Copy link
Contributor Author

@Ch3LL I would like to get this merged. What steps remain?

2 similar comments
@amendlik
Copy link
Contributor Author

@Ch3LL I would like to get this merged. What steps remain?

@amendlik
Copy link
Contributor Author

amendlik commented Feb 1, 2021

@Ch3LL I would like to get this merged. What steps remain?

krionbsd
krionbsd previously approved these changes Feb 12, 2021
@sagetherage
Copy link
Contributor

re-running the failed windows test to see if it is flakey or not, will check back later today on results

@sagetherage sagetherage added the Aluminium Release Post Mg and Pre Si label Feb 22, 2021
Ch3LL
Ch3LL previously approved these changes Feb 23, 2021
@Ch3LL Ch3LL requested a review from s0undt3ch February 23, 2021 11:31
@amendlik amendlik dismissed stale reviews from Ch3LL and krionbsd via 2265644 February 24, 2021 15:38
@Ch3LL Ch3LL merged commit 3d517b0 into saltstack:master Feb 25, 2021
@amendlik amendlik deleted the module-run-state branch May 7, 2021 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants