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

device: Enable cisco-vrf discovery for junos os #8033

Merged
merged 2 commits into from Jan 17, 2018

Conversation

Projects
None yet
5 participants
@zombah
Contributor

zombah commented Jan 5, 2018

As cisco-vrf now works fine with Junos os it seems fine
to enable it by default

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@laf

This comment has been minimized.

Member

laf commented Jan 5, 2018

Bad module name then if it's not cisco specific :(

I think we should update this to be just vrf, that will mean file name changes, doc updates, a check for the old config variable in includes/process_config.inc.php and set the new one to true if the old one was and then discover_cisco-vrf updating to discover_vrf in the devices_attribs table to that db selected modules work.

It's a big ask so if you don't fancy it, let me know and I'll do that.

@zombah

This comment has been minimized.

Contributor

zombah commented Jan 5, 2018

Agree. Sure i'll do. VRF is specific for Cisco and Arista, maybe to choose vendor agnostic name, like mib name, mpls-vpn or mpls-l3vpn?

@laf

This comment has been minimized.

Member

laf commented Jan 6, 2018

You pick, basically it just needs to represent what it does now seeing as it's no longer cisco vrf specific.

@Zmegolaz

This comment has been minimized.

Member

Zmegolaz commented Jan 8, 2018

I'd say "VRF" is fine, it's not vendor specific and is used in Juniper too. ("set instance-type vrf")

@zombah

This comment has been minimized.

Contributor

zombah commented Jan 8, 2018

Yes i'll keep vrf, i found that it is used alot in webui,docs and etc places so i see no reason to create segmentation with other term variants.

@laf

laf approved these changes Jan 8, 2018

LGTM on this one.

@librenms/reviewers anyone else fancy checking this?

@murrant

This comment has been minimized.

Member

murrant commented Jan 15, 2018

Looks good to me too. (obviously needs the schema to be moved up)

zombah added some commits Jan 5, 2018

device: Enable cisco-vrf discovery for junos os
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
refactor: Rename cisco-vrf module into vrf
Signed-off-by: Misha Komarovskiy <zombah@gmail.com>
@zombah

This comment has been minimized.

Contributor

zombah commented Jan 15, 2018

Rebased versus current master

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 15, 2018

The inspection completed: No new issues

@laf laf merged commit df731c1 into librenms:master Jan 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Member

laf commented Jan 17, 2018

Awesome work @zombah. Thanks for going through all of that :D

@zombah zombah deleted the zombah:junos-cisco-vrf branch Jan 17, 2018

@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.