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

remove apc-ats os and merge sensors into apc #9262

Merged
merged 7 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@tomarch
Copy link
Contributor

tomarch commented Sep 25, 2018

Thanks @FTBZ for the #9221 !
But i think we don't need a new os "apc-ats" because:
-it need to add exception in "apc" os definition (ATS use same firmware and so the same Sysdesc)
-ATS have other sensor which is already present in "apc" discovery

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.

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 25, 2018

@FTBZ Any comments?

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Sep 26, 2018

On my side the metrics are returning errors and are not working. The problem is that to fix these metrics, we risk breaking things for PDU users. That's why I created the new OS.

After if the metrics do not work for me, it does not matter as long as I have the state sensors.

@tomarch

This comment has been minimized.

Copy link
Contributor Author

tomarch commented Sep 26, 2018

I have some issue with metric before too (voltage and current was at 0), but it has been resolved with the lastest apc firmware (6.5.6).
Now i have all the sensors workings :
capture d ecran de 2018-09-26 08-20-04

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Sep 26, 2018

Nice to know, seems better now with the new firmware.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Sep 26, 2018

Instead of deleting the test data, rename it to APC (and update it if necessary)

@tomarch

This comment has been minimized.

Copy link
Contributor Author

tomarch commented Sep 27, 2018

I've add the test data.
thanks.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Sep 27, 2018

You need the snmprec too. I would suggest the name apc_ats.snmprec and apc_ats.json for the files.

@tomarch tomarch force-pushed the tomarch:merge_apc-ats_into_apc branch from 7861a4d to 7f82aff Sep 27, 2018

@tomarch

This comment has been minimized.

Copy link
Contributor Author

tomarch commented Sep 27, 2018

We want to remove apc-ats OS as we don't need it.
There's already a apc.snmprec, so i only remove apc-ats.snmprec

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Sep 27, 2018

yeah the underscore separates the os and the variant, a dash means it is part of the os name.

apc-ats (OS is apc-ats)
apc_ats (OS is apc, variant is ats)

We use variant to allow multiple test datas per os.

@tomarch

This comment has been minimized.

Copy link
Contributor Author

tomarch commented Sep 28, 2018

OK. thanks for this information.

@laf laf added the Device 🖥 label Oct 3, 2018

@laf

This comment has been minimized.

Copy link
Member

laf commented Oct 3, 2018

Updated tests.

@laf

laf approved these changes Oct 13, 2018

Copy link
Member

laf left a comment

LGTM

@laf laf merged commit 8005619 into librenms:master Oct 17, 2018

3 checks passed

WIP ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details

murrant added a commit that referenced this pull request Oct 18, 2018

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Oct 28, 2018

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

https://community.librenms.org/t/v1-45-release-changelog-october-2018/6016/1

@lock lock bot locked as resolved and limited conversation to collaborators Dec 28, 2018

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.