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

Added support for Brocade 200E #5617

Merged
merged 16 commits into from Jan 27, 2017

Conversation

Projects
None yet
4 participants
@aldemira
Contributor

aldemira commented Jan 26, 2017

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.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

@aldemira aldemira changed the title from Add support for Brocade 200E to Added support for Brocade 200E Jan 26, 2017

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 26, 2017

Auto-Deploy finished, Test PR at http://5617.ci.librenms.org or https://5617.ci.librenms.org

@laf

This comment has been minimized.

Member

laf commented Jan 26, 2017

You need to also add the extra test call into tests/OSDiscoveryTest.php

@laf

This comment has been minimized.

Member

laf commented Jan 26, 2017

Also, whilst you are at it, can you change your github name in AUTHORS.md to aldemira rather than aldemir_a.

It's also worth to mention that you should avoid working in your master branch. We have docs to help you with that :)

aldemira added some commits Jan 26, 2017

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 26, 2017

Auto-Deploy finished, Test PR at http://5617.ci.librenms.org or https://5617.ci.librenms.org

1 similar comment
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 26, 2017

Auto-Deploy finished, Test PR at http://5617.ci.librenms.org or https://5617.ci.librenms.org

@laf

This comment has been minimized.

Member

laf commented Jan 26, 2017

Sorry my head isn't with it :(

Can you add sysDescr to the test unit file, it's not used for discovery but helps with the whole unit testing piece in general.

@aldemira

This comment has been minimized.

Contributor

aldemira commented Jan 26, 2017

I would, but sysDescr string says "Fibre Channel Switch."
I think it's a bit too generic, don't you think so?

@laf

This comment has been minimized.

Member

laf commented Jan 26, 2017

Not really, yes it is generic but if someone adds a new device which only matches on Fibre Channel sysDescr and it runs before this one then it will break detection. The more test data we have the better :)

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 27, 2017

Auto-Deploy finished, Test PR at http://5617.ci.librenms.org or https://5617.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 27, 2017

The inspection completed: No new issues

@laf laf merged commit 46003a3 into librenms:master Jan 27, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment