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

Two more options added to the netbox Pillar, and use salt.utils.http #48910

Merged
merged 3 commits into from Aug 5, 2018

Conversation

Projects
None yet
5 participants
@mirceaulinic
Copy link
Member

commented Aug 3, 2018

What does this PR do?

I am applying some small (or I think so) changes to the netbox external Pillar:

  • Use salt.utils.http instead of requests to avoid external dependency.
  • Add two new options: site_details and site_prefixes.
  • versionadded tag.

@ksofa2 @ngrundler I hope you don't mind my changes, please let me know if there's anything you disagree / should be changed etc. Additionally, it would be great if you could test this out and confirm it doesn't change anything in your environment. Thanks!

@mirceaulinic mirceaulinic requested a review from saltstack/team-core as a code owner Aug 3, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Aug 3, 2018

@ngrundler

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

@mirceaulinic

No issues on my end, this looks great. A good learning example for me about how I can use features built into salt to remove dependencies and make things simpler!

What do you think about nesting the different NetBox API endpoints into the netbox pillar key, e.g.

"netbox": {
    "device": { <device details> },
    "site": { <site details> },
    "interfaces": { <interface details> },
    "ip-addresses": { <ip address details> }
 }

I've since made some tweaks internally since we're trying to use netbox data to drive configuration templates and I needed this data available, so I have set up queries to interfaces, addresses, connections, etc. I was planning on adding this to the module myself soon(ish), but I see no reason for that to impact this PR.

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2018

What do you think about nesting the different NetBox API endpoints into the netbox pillar key, e.g.

That would make sense to me, yes.

mirceaulinic added some commits Aug 3, 2018

@Ichabond

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

What do you think about nesting the different NetBox API endpoints into the netbox pillar key, e.g.

I think that's a great idea, but I'd nest them in the App endpoint as well. So instead of the flatter structure you proposed, I propose:

"netbox": {
    "dcim": {
        "device": { <device details> },
        "site": { <site details> },
        "interfaces": { <interface details> }        
    },
    "ipam": {
        "ip-addresses": { <ip address details> }
    }
 }

This will make sure there's no conflicts if/when Netbox has 2 apps with identical API endpoints.

@rallytime rallytime added the Fluorine label Aug 3, 2018

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2018

I think these proposals make sense to me, I would leave it though for another day to discuss this further. Wondering if @zachmoody has any thoughts on this?

@zachmoody

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

TIL that salt.utils.http doesn't use requests, by default anyways. That's impressive. 😄

I'd definitely nest under app though, ran into the same conundrum with earlier versions of pynetbox and in the end had to acquiesce to a two-tiered hierarchy.

@rallytime
Copy link
Contributor

left a comment

Nice

@rallytime rallytime merged commit c034a86 into saltstack:develop Aug 5, 2018

6 of 9 checks passed

codeclimate 3 issues to fix
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details

@mirceaulinic mirceaulinic deleted the mirceaulinic:netbox-pillar branch Aug 9, 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.