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

Depends decorator version master #55590

Merged
merged 12 commits into from Dec 27, 2019

Conversation

github-abcde
Copy link
Contributor

@github-abcde github-abcde commented Dec 10, 2019

This is #53658, rebased on master.

What does this PR do?

It adds a version kwarg to the @Depends decorator, which can handle any value that is able to be parsed with salt.utils.version.LooseVersion. The value of this version kwarg will be compared to the __version__-attribute of the module mentioned in the decorator (if it cannot be found (because it is a command or the module does not have the attribute), it will silently default to None). If the version is not equal or larger than the determined version (noting that when the latter is None, everything will be either equal (also None) or greater (any value)), the specific function will be removed by enforce_dependencies.

What issues does this PR fix or reference?

None that I know of

Previous Behavior

The @Depends decorator can only specify whether or not a module needs to be loaded.

New Behavior

The @Depends decorator can also specify a specific minimum version for the module that needs to be loaded.

Tests written?

No, there are tests in tests/integration/modules/test_decorators.py, but I have no idea how those work.

Commits signed with GPG?

Yes

Fixed logic path when version is insufficient.
Added version parameter in unloading log message if used.

log.trace(
'Unloading %s.%s because dependency (%s) is not met',
mod_name, func_name, dependency
'Unloading %s.%s because dependency (%s%s) is not met',
Copy link
Contributor

@waynew waynew Dec 12, 2019

Choose a reason for hiding this comment

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

Was this missing a .?

Suggested change
'Unloading %s.%s because dependency (%s%s) is not met',
'Unloading %s.%s because dependency (%s.%s) is not met',

Copy link
Contributor Author

@github-abcde github-abcde Dec 12, 2019

Choose a reason for hiding this comment

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

Nope, the 4th %s placeholder is filled with https://github.com/saltstack/salt/pull/55590/files#diff-7bd4f0af6512326b8c5a14e4ecb70b40R211 which starts with a space. Alternatively, I could move the space to this line (207) if you prefer.

@waynew
Copy link
Contributor

@waynew waynew commented Dec 12, 2019

Hey, thanks for this PR!

I went a-looking to see what I could see Re: tests:

In tests/integration/files/file/base/_modules/runtests_decorators.py there are a bunch of functions that have been decorated. The tests in tests/integration/modules/test_decorators.py use salt to call those functions when testing to make sure that they do the right thing.

I think similar functions could be added to the existing depends ones in there. In fact, I think adding an additional depends (or something) module in there (tests/integration/files/file/base/_modules) with a __version__ attribute would make it possible to test that the version check works as intended. Though might need a verisionless_depends as well, given the check for __version__ in this PR.

Let me know if you need any more help on this!

dwoz
dwoz approved these changes Dec 27, 2019
@dwoz dwoz merged commit 85178d4 into saltstack:master Dec 27, 2019
49 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.

None yet

3 participants