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

Update Netbox execution module #48916

Merged
merged 3 commits into from
Aug 8, 2018
Merged

Conversation

Ichabond
Copy link
Contributor

@Ichabond Ichabond commented Aug 3, 2018

What does this PR do?

Add multiple functions to interact with the Netbox API.

New Behavior

This PR adds more functions to the Netbox execution module. This further expands the functionality by allowing deletion and creation of Netbox objects as well.

Tests written?

No

Commits signed with GPG?

No

CC: @zachmoody @mirceaulinic

if id:
item = getattr(getattr(nb, app), endpoint).get(id)
else:
item = getattr(getattr(nb, app), endpoint).get(**salt.utils.args.clean_kwargs(**kwargs))
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using salt.utils.args.clean_kwargs, it is already injected in these modules, just use __utils__['args.clean_kwargs'](**kwargs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this, but this breaks the unit tests:

Traceback (most recent call last):
  File "/Users/tstrickx/Library/Python/2.7/lib/python/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/Users/tstrickx/Documents/Projects/salt/tests/unit/modules/test_netbox.py", line 86, in test_token_present
    netbox.get_('dcim', 'devices', name='test')
  File "/Users/tstrickx/Documents/Projects/salt/salt/modules/netbox.py", line 191, in get_
    return _dict(_get(app, endpoint, id=id, auth_required=True if app in AUTH_ENDPOINTS else False, **kwargs))
  File "/Users/tstrickx/Documents/Projects/salt/salt/modules/netbox.py", line 128, in _get
    kwargs = __utils__['args.clean_kwargs'](**kwargs)
KeyError: u'args.clean_kwargs'

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using this on 2017.7 or older? because the args.py file was added starting in 2018.3.

Copy link
Contributor

Choose a reason for hiding this comment

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

you would need to include the args.py in your salt://_utils as well for older versions.

Copy link
Contributor Author

@Ichabond Ichabond Aug 3, 2018

Choose a reason for hiding this comment

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

This happened while running the unit tests using ./runtests.py --name=unit.modules.test_netbox. Not sure how that relates to which version of Salt I'm running.
Maybe this is relevant?

pytest-salt (2018.2.2)          - Pytest Salt Plugin
  INSTALLED: 2018.2.2 (latest)

}
}
}
oc_if[if_name] = salt.utils.dictupdate.update(oc_if[if_name], subif_descr)
Copy link
Contributor

Choose a reason for hiding this comment

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

__utils__['dictupdate.update']()

return if_name, '0'


def filter_(app, endpoint, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

We like to avoid the use of **kwargs because it's unclear to the end user what it supposed to be passed here. We either need this made into an explicit variable or an explanation of what's expected here inserted into the function documentation, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


salt myminion netbox.create_device edge_router router MX480 Juniper BRU
'''
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated in a few places. Can we refactor it into a single location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understood, you want to refactor the "does this object exist in netbox" checks to a single location. Most of these checks are single use, except for the device one, which I guess could be put in a function. Is this what you meant?

nb_device = _get('dcim', 'devices', auth_required=True, name=name)
for k, v in kwargs.items():
setattr(nb_device, k, v)
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. DRY?

if not nb_man:
create_manufacturer(manufacturer_name)
nb_man = get_('dcim', 'manufacturers', name=manufacturer_name)
if not serial:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just make the empty strings the keyword defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Ichabond Ichabond force-pushed the develop branch 4 times, most recently from bf22380 to 10b9ec5 Compare August 6, 2018 14:56
Add multiple functions to interact with the Netbox API.
@Ichabond
Copy link
Contributor Author

Ichabond commented Aug 6, 2018

I have made the requested changes, and updated the tests to function. Please let me know if there's anything else that needs to be done before merging.

Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for updating this.

@Ichabond Ichabond changed the base branch from develop to fluorine August 7, 2018 13:54
@Ichabond
Copy link
Contributor Author

Ichabond commented Aug 7, 2018

I changed the branch to Fluorine. CC @rallytime

@rallytime
Copy link
Contributor

Thanks @Ichabond! And congrats on your first Salt PR!

@rallytime rallytime merged commit 4f6eccd into saltstack:fluorine Aug 8, 2018
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

4 participants