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

Allow for passing in previously compiled grains to custom grain modules #47372

Merged
merged 3 commits into from May 29, 2018

Conversation

Projects
None yet
5 participants
@gtmanfred
Copy link
Contributor

commented Apr 27, 2018

What does this PR do?

If a grains module function accepts a variable named grains then the grains dict will be passed in.

What issues does this PR fix or reference?

Closes #47338

Previous Behavior

No grains are passed to custom grains modules

New Behavior

Previously compiled grains are passed into grains functions if the function accepts a variable called grains

Tests written?

Yes

Commits signed with GPG?

Yes

@gtmanfred gtmanfred requested a review from saltstack/team-core as a code owner Apr 27, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Apr 27, 2018

if funcs[key].__code__.co_argcount == 1:
ret = funcs[key](proxy)
parameters = list(funcs[key].__code__.co_varnames)
if funcs[key].__code__.co_argcount > 0:

This comment has been minimized.

Copy link
@isbm

isbm Apr 28, 2018

Contributor

The > 0 is not really needed. :-) Enough here if parameters:. But the whole check is also not needed (see below).

kwargs['proxy'] = proxy
if 'grains' in parameters:
kwargs['grains'] = grains_data
ret = funcs[key](**kwargs)

This comment has been minimized.

Copy link
@isbm

isbm Apr 28, 2018

Contributor

No need to double-check length of the co_varnames as well as no need to if/else to call funcs[key] with or without kwargs. If parameters is an empty list, kwargs stays that way too, and so the call funcs[key](**{}) equals to funcs[key]() as so:

parameters = list(funcs[key].__code__.co_varnames)
kwargs = {}
if 'proxy' in parameters:
    kwargs['proxy'] = proxy
if 'grains' in parameters:
    kwargs['grains'] = grains_data
ret = funcs[key](**kwargs)

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Apr 28, 2018

Author Contributor

good point, and it looks like you can pass an empty kwargs to a function that doesn't take any arguments, TIL

'''
test if current grains are passed to grains module functions that have a grains argument
'''
self.assertEqual(self.run_function('grains.get', ['custom_grain_test']), 'itworked')

This comment has been minimized.

Copy link
@isbm

isbm Apr 28, 2018

Contributor

'cmon, Dani, use PyTest, let's drop this camel case finally! 😉 Just assert that.

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Apr 28, 2018

Author Contributor

We are still using Unittests, so I am going to stick to unittests asserts until that changes.

This comment has been minimized.

Copy link
@isbm

isbm Apr 30, 2018

Contributor

Actually, PyTest is now the next thing and we already running it all over the place anyway. Why would one invest in soon-to-be-dead old stuff and thus create another technical debt that will cause rewrite in the near distant future? I wouldn't do that.

For custom grains, if the function takes an argument ``grains``, then the
previously rendered grains will be passed in. Because the rest of the grains
could be rendered in any order, the only grains that can be relied upon to be
passed in are ``core`` grains. This was added in the Fluorine Release.

This comment has been minimized.

Copy link
@isbm

isbm Apr 28, 2018

Contributor

Typo. "Passed in a core grains", I believe.

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Apr 28, 2018

Author Contributor

that is not correct.

This comment has been minimized.

Copy link
@cachedout

cachedout May 8, 2018

Collaborator

Release should not be capitalized though. ;)

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

re-run py

For custom grains, if the function takes an argument ``grains``, then the
previously rendered grains will be passed in. Because the rest of the grains
could be rendered in any order, the only grains that can be relied upon to be
passed in are ``core`` grains. This was added in the Fluorine Release.

This comment has been minimized.

Copy link
@cachedout

cachedout May 8, 2018

Collaborator

Release should not be capitalized though. ;)


Starting in this release, if a custom grains function accepts a variable named
``grains``, the Grains dictionary of the already compiled grains will be passed
in. Because of the non deterministic order that grains are rendered in, the

This comment has been minimized.

Copy link
@cachedout

cachedout May 8, 2018

Collaborator

non-deterministic

@gtmanfred

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

re-run py

@gtmanfred gtmanfred merged commit 45da45f into saltstack:develop May 29, 2018

5 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5293 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23227 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10264 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19349 — ABORTED
Details
default Build finished.
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25483 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17056 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21676 — SUCCESS
Details

@gtmanfred gtmanfred deleted the gtmanfred:grains branch May 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.