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

states/netyang.managed ignores models when passed as a kwarg #47425

Open
raddessi opened this issue May 2, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@raddessi
Copy link

commented May 2, 2018

Description of Issue/Question

I'm not sure why it's being passed as a kwarg but it does appear to be happening. In the code I have locally this is the function def:

def managed(name,
            data,
            *models,
            **kwargs):
...
    if isinstance(models, tuple) and isinstance(models[0], list):
        models = models[0]

So if models=('blah',) gets passed in it will be put in to kwargs and the first line of the function will fail with an IndexError:

May 01 18:24:35 <hostname> bash[31493]: [ERROR   ] An exception occurred in this state: Traceback (most recent call last):
May 01 18:24:35 <hostname> bash[31493]:   File "/opt/miniconda/envs/salt/lib/python2.7/site-packages/salt/state.py", line 1878, in call
May 01 18:24:35 <hostname> bash[31493]:     **cdata['kwargs'])
May 01 18:24:35 <hostname> bash[31493]:   File "/opt/miniconda/envs/salt/lib/python2.7/site-packages/salt/loader.py", line 1823, in wrapper
May 01 18:24:35 <hostname> bash[31493]:     return f(*args, **kwargs)
May 01 18:24:35 <hostname> bash[31493]:   File "/opt/miniconda/envs/salt/lib/python2.7/site-packages/salt/states/netyang.py", line 146, in managed
May 01 18:24:35 <hostname> bash[31493]:     if isinstance(models, tuple) and isinstance(models[0], list):
May 01 18:24:35 <hostname> bash[31493]: IndexError: tuple index out of range

Setup

This is reproducible with the example setup instructions from https://docs.saltstack.com/en/latest/ref/states/all/salt.states.netyang.html#salt.states.netyang.managed

Steps to Reproduce Issue

Same as above

To fix the issue, I added one line before the current type check:

def managed(name,
            data,
            *models,
            **kwargs):
...
    models = kwargs.get('models', models)  # new
    if isinstance(models, tuple) and isinstance(models[0], list):
        models = models[0]

Versions Report

Salt Version:
Salt: 2018.3.0

Dependency Versions:
cffi: 1.11.5
cherrypy: unknown
dateutil: Not Installed
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.9.6
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.5.1
mysql-python: Not Installed
pycparser: 2.18
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.14 |Anaconda, Inc.| (default, Nov 8 2017, 22:44:41)
python-gnupg: 0.4.1
PyYAML: 3.12
PyZMQ: 16.0.3
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.1.6

System Versions:
dist: fedora 26 Twenty Six
locale: UTF-8
machine: x86_64
release: 4.15.17-200.fc26.x86_64
system: Linux
version: Fedora 26 Twenty Six

@gtmanfred gtmanfred added this to the Approved milestone May 2, 2018

@gtmanfred gtmanfred added the Bug label May 2, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

@mirceaulinic when you get back from vacation, if you could check if this fix is correct.

Thanks,
Daniel

@mirceaulinic

This comment has been minimized.

Copy link
Member

commented May 9, 2018

That change seems to have been introduced in 2e930f7, we should probably just revert that commit. @raddessi would you be able to check that in your environment (just copy a previous version, say https://github.com/saltstack/salt/blob/6fad40454ae19330ec8fd060493c3ada5ef67a60/salt/states/netyang.py in your salt://_states directory).

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

I think we should also have @tonybaloney take a look at this then.

@raddessi

This comment has been minimized.

Copy link
Author

commented Jun 4, 2018

Any chance you've been able to take a look @tonybaloney?

tonybaloney added a commit to tonybaloney/salt that referenced this issue Oct 30, 2018

@tonybaloney

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

@raddessi @mirceaulinic added fixes and simple tests in #50301

rallytime added a commit that referenced this issue Oct 30, 2018

Merge pull request #50301 from tonybaloney/issue_47425
Fixes issues raised in #47425 for states.netyang

rallytime added a commit to rallytime/salt that referenced this issue Oct 30, 2018

@tonybaloney

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

Fixed in merged PR, please verify @raddessi @mirceaulinic

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.