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

newdevice: Added detection for Hirschmann Railswitch #6161 #6207

Merged
merged 16 commits into from Mar 23, 2017

Conversation

Projects
None yet
6 participants
@mail4git38
Contributor

mail4git38 commented Mar 16, 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?

Testers

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

mail4git38 added some commits Mar 16, 2017

Update AUTHORS.md
I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.

@mail4git38 mail4git38 changed the title from Added detection for Hirschmann Railswitch to Added detection for Hirschmann Railswitch #6161 Mar 16, 2017

@mail4git38 mail4git38 changed the title from Added detection for Hirschmann Railswitch #6161 to newdevice: Added detection for Hirschmann Railswitch #6161 Mar 16, 2017

@murrant

Thanks for your contribution. Just a few changes and it should be good.

Do you have a better icon? SVG is preferred.

@@ -0,0 +1,7 @@
<?php
$hardware = trim(snmp_get($device, "hmPNIOOrderID.0", "-OQv", "HMPRIV-MGMT-SNMP-MIB"), '"');

This comment has been minimized.

@murrant

murrant Mar 17, 2017

Member

Is trim() needed here? or any of the lines below?

This comment has been minimized.

@mail4git38

mail4git38 Mar 20, 2017

Contributor

trim() is indeed not needed here and in the lines below. I remove it.

@@ -0,0 +1,7 @@
<?php

This comment has been minimized.

@murrant

murrant Mar 17, 2017

Member

Remove this file, it is handled by the yaml.

This comment has been minimized.

@mail4git38

mail4git38 Mar 20, 2017

Contributor

This file is an old file from a previous LibreNMS installation. I've removed it.

- { graph: device_processor, text: 'CPU Usage' }
- { graph: device_mempool, text: 'Memory Usage' }
discovery:
- sysDescr:

This comment has been minimized.

@murrant

murrant Mar 17, 2017

Member

Please use sysObjectId instead of sysDescr.

This comment has been minimized.

@mail4git38

mail4git38 Mar 20, 2017

Contributor

sysDescr is now replaced by sysObjectId.

@mail4git38

This comment has been minimized.

Contributor

mail4git38 commented Mar 20, 2017

@murrant

Sorry, I'm a new user with git :-(

The requested changes are done on my dev server but I can't push my commits to issue-6161.

Here is the message I have : 'updates were rejected because the tip of your current branch is behind its remote counterpart' when I execute 'git push origin issue-6161' command.

How can I push my changes ?

@murrant

This comment has been minimized.

Member

murrant commented Mar 22, 2017

@mail4git38 sorry for the late response. Run git pull first to pull down remote changes.

Sorry about that, I was fixing merge conflict.

@mail4git38

This comment has been minimized.

Contributor

mail4git38 commented Mar 22, 2017

Hello @murrant. Thanks for your response and no problem with the delay.

I've tried to run git pull upstream master command before git push origin issue-6161 but I still have the same issue (rejected updates).

@laf

This comment has been minimized.

Member

laf commented Mar 22, 2017

You need to run git pull or if that doesn't work git pull origin issue-6161 (i've assumed your remote is called origin - replace that name if it's not)

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Mar 22, 2017

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

1 similar comment
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Mar 22, 2017

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

@mail4git38

This comment has been minimized.

Contributor

mail4git38 commented Mar 22, 2017

Thanks a lot for your help on git for my first contribution.

I successfully pushed my new commits to the issue 6161.

@laf laf added the Blocker 🚫 label Mar 22, 2017

@laf

This comment has been minimized.

Member

laf commented Mar 22, 2017

Only two things from me (aside from the merge conflict).

You have two files for state, one in includes/discovery/sensors/states/ which is the old location and needs to be deleted.

The other is you've got the same $index value in all of the state support so this shouldn't be working. You would be better off updating these to use name.$index ie.: hmPowerSupplyStatus.0, hmPowerSupplyStatus.1, etc.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Mar 22, 2017

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

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Mar 22, 2017

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

@laf

This comment has been minimized.

Member

laf commented Mar 22, 2017

Just the issue with state indexes now and then good for a merge.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Mar 23, 2017

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

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 23, 2017

The inspection completed: 1 updated code elements

@murrant murrant merged commit bc14b91 into librenms:master Mar 23, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@murrant murrant referenced this pull request Mar 23, 2017

Closed

Added detection for Hirschmann Railswitch #6161

5 of 5 tasks complete
@laf

This comment has been minimized.

Member

laf commented Mar 23, 2017

@mail4git38 Thanks for submitting this. Unfortunately you've removed yourself from the AUTHORS.md at the end of this PR. Please submit a new pull request adding yourself back in and agreeing to the contributors agreement otherwise we will have to revert this pull request.

@mail4git38 mail4git38 deleted the mail4git38:issue-6161 branch Mar 24, 2017

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