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

Added support Ericsson MINI-LINK #10546

Merged
merged 17 commits into from Aug 22, 2019

Conversation

@erotel
Copy link
Contributor

commented Aug 21, 2019

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
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@erotel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I opened it again because I did the previous one wrong

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Hello,
It looks very good. The temperature sensor file includes/discovery/sensors/temperature/ericsson-ml.inc.php could have been done via YAML but this php version works as well.

Could you run the ./scripts/collect-snmp-data.php -h <yourDeviceId> (it will update the snmpsim with the temperature and wireless data) and ./scripts/save-test-data.php -o ericsson-ml and attach the resulting file to the PR ? that would make the PR complete.
Let me know if you need help for this step.

@PipoCanaja
Copy link
Contributor

left a comment

A little bit of housecleaning in the MIB files is required to avoid duplicates, as well as the tests thing I mentionned in the comments already, and this will be good to go.
Thanx :)

mibs/ericsson/PT-ALL-MIB Outdated Show resolved Hide resolved
mibs/ericsson/PT-TRAP-ALL-MIB Outdated Show resolved Hide resolved
@erotel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

sorry I'm git confused about this, I don't know why I put .gitignore files in there

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

I did some cleaning in the files that you removed by mistake. We'll see if it is better now.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

sorry I'm git confused about this, I don't know why I put .gitignore files in there

The file permissions were changed, that's why they appeared here. But deleting them from here is not the correct solution. It means deleting them from librenms alltogether.

In order to cancel a change in a Pull Request (and revert to the file as it is in 'master') you can do :
git checkout master path/to/your/file

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@erotel : Change is now approved. If it is OK with you, I can merge the PR.

@erotel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

for me everything is fine

@PipoCanaja PipoCanaja merged commit f16805b into librenms:master Aug 22, 2019

5 of 6 checks passed

codeclimate 2 issues to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.