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 for East iStars UPS (os: istars) #10041

Merged
merged 11 commits into from May 21, 2019

Conversation

Projects
None yet
4 participants
@spencerbutler
Copy link
Contributor

commented Mar 28, 2019

Fixes:
#8480

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.

@PipoCanaja PipoCanaja added this to the 1.51 milestone Mar 28, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Hi @spencerbutler
This one is the one that impelements rfc1628 badly, is it ?
Let me know if you have some tests done by requester or if we merge it right away.
Bye

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Hi @spencerbutler
This one is the one that impelements rfc1628 badly, is it ?
Let me know if you have some tests done by requester or if we merge it right away.
Bye

No feedback from the requester, as of yet. Let's wait a bit to see if any comes in.

@PipoCanaja
Copy link
Contributor

left a comment

@spencerbutler If the image files are the same, you can put only the 'os' and it will be always used, instead of duplicating the file (and the space used).

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@spencerbutler If the image files are the same, you can put only the 'os' and it will be always used, instead of duplicating the file (and the space used).

ah, good advice. thanks and fixed.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

LGTM, any feedback from the requester ?

@murrant
Copy link
Member

left a comment

Instead of redefining UPS-MIB sensors, use this in the os yaml:

rfc1628_compat: true
@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Instead of redefining UPS-MIB sensors, use this in the os yaml:

rfc1628_compat: true

fixed

spencerbutler and others added some commits Apr 8, 2019

@@ -7,7 +7,7 @@
foreach ($load_data as $index => $data) {
$load_oid = ".1.3.6.1.2.1.33.1.4.4.1.5.$index";
if (is_array($data['upsOutputPercentLoad'])) {
if (isset($data['upsOutputPercentLoad'][0])) {

This comment has been minimized.

Copy link
@laf

laf Apr 9, 2019

Member

Any reason this was changed?

This comment has been minimized.

Copy link
@spencerbutler

spencerbutler Apr 9, 2019

Author Contributor

Yes, @murrant suggested it was a better way to check, so I updated all references to the upsDataFoo from is_array to isset.

@laf

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

The tests are still failing: https://travis-ci.com/librenms/librenms/jobs/191452775

This looks like it's because it's affecting the existing data for sensors - are you sure this is backwards compatible?

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

The tests are still failing: https://travis-ci.com/librenms/librenms/jobs/191452775

This looks like it's because it's affecting the existing data for sensors - are you sure this is backwards compatible?

I will investigate this further.

@murrant

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Hmm, looks like your changes to rfc1628.inc.php broke some other devices.

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Hmm, looks like your changes to rfc1628.inc.php broke some other devices.

Yeah, I'll see if I can fix that. Otherwise, I'll just revert the changes I made outside the scope of this PR.

@PipoCanaja PipoCanaja modified the milestones: 1.51, 1.52 Apr 21, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@spencerbutler Still working on this?

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@spencerbutler Still working on this?

I've been out of play, but I do intend on fixing this up. I'll make an effort to finish it this week.

@murrant

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@spencerbutler your changes to all the rfc1628 files breaks everything. :) Either revert them or change it so it doesn't break other devices.

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@spencerbutler your changes to all the rfc1628 files breaks everything. :) Either revert them or change it so it doesn't break other devices.

Reverted.

@@ -34,6 +34,9 @@
if (count($output_current) > 1) {
$descr .= " Phase $index";
}
if (is_array($data['upsOutputCurrent'])) {

This comment has been minimized.

Copy link
@murrant

murrant May 21, 2019

Member

Other rfc1628 types have this code block:

    if (is_array($data['upsOutputVoltage'])) {
        $upsOutputVoltage_value = $data['upsOutputVoltage'][0];
        $volt_oid .= ".0";
    }

You should probably update the oid being discovered as well.

"sensor_index": "3.2.0.1",
"sensor_type": "rfc1628",
"sensor_descr": "Input",
"group": null,
"sensor_divisor": 10,
"sensor_multiplier": 1,
"sensor_current": 0,
"sensor_current": 60,

This comment has been minimized.

Copy link
@murrant

murrant May 21, 2019

Member

Hey look, your test data works :)

Stale review

@murrant murrant removed the Needs Testing label May 21, 2019

@murrant murrant merged commit 83522c6 into librenms:master May 21, 2019

5 of 6 checks passed

codeclimate 6 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

@lock lock bot locked as resolved and limited conversation to collaborators Jul 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.