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.

Sign up for GitHub

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

Jump to bottom

Extend Avocent Support #14914

Merged
merged 32 commits into from Jun 15, 2023
Merged

Extend Avocent Support #14914

merged 32 commits into from Jun 15, 2023

Conversation

electrocret
Copy link
Member

@electrocret electrocret commented Mar 20, 2023 •
edited

Please give a short description what your pull request is for

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

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.

@Jellyfrog Jellyfrog added the Device 🖥️ New or added device support label Mar 20, 2023
@electrocret electrocret marked this pull request as ready for review April 4, 2023 21:27
@murrant murrant added the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Apr 7, 2023
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Please add test data so we can ensure your change is not broken in the future.
Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

@electrocret
Copy link
Member Author

electrocret commented Apr 19, 2023 •
edited

Updated existing tests for ACS6000 with new datapoints.

@murrant
Copy link
Member

murrant commented Apr 20, 2023

Question. If you deleted the previous test case, how do we know you didn't break it or what changed?

@electrocret
Copy link
Member Author

electrocret commented Apr 21, 2023 •
edited

-None of the existing YAML was changed, so no less data would be collected.
-Both tests were performed on an ACS6048, so it's not a variant and existing data points should be there, just different values cause they've been configured/used differently by the user.

Would we prefer to have multiple tests for the same device?

@murrant
Copy link
Member

murrant commented Apr 24, 2023

No and yes. More variants = tests take more time for possibly no gain.
But it is extremely difficult if not very time consuming to determine if the new test data actually covers all the same cases as the existing data.

One way to do it would be to separate the code and tests data consolidation. That way with no code changes, the new test data is more likely to be correct.

Minimizing changes in the snmprec can be another good way to reduce noise. But has most of the same issues and could be just invalid data with the other data.

@murrant
Copy link
Member

murrant commented Apr 24, 2023

FYI, this PR is likely fine, just taking a second to reflect on this.

@Jellyfrog Jellyfrog closed this Apr 30, 2023
@Jellyfrog Jellyfrog reopened this Apr 30, 2023
@Jellyfrog Jellyfrog removed the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Apr 30, 2023
@electrocret
Copy link
Member Author

@murrant Bump?

@electrocret electrocret merged commit b0ddd4e into librenms:master Jun 15, 2023
14 checks passed
@electrocret electrocret deleted the Extend-Avocent branch June 15, 2023 03:50
@librenms-bot
Copy link

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

https://community.librenms.org/t/23-6-0-changelog/21693/1

TheMysteriousX pushed a commit to TheMysteriousX/librenms that referenced this pull request Aug 9, 2023
@electrocret @TheMysteriousX
Extend Avocent Support (librenms#14914)
ddb405f 
* Add files via upload

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Rename ACS6000-MIB to ACS-MIB

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Update avocent.yaml

* Create avocent_acs6048.snmprec

* Create avocent_acs6048.json

* Delete avocent_acs6048.json

* Same as avocent_6000

* Point at ACS MIB

ACS-MIB::acsSerialPortTableName

* Update avocent_6000.snmprec

Update avocent_6000 with additional MIB

* Update Test data

* Update avocent_6000.snmprec

* Update avocent_6000.snmprec

* Update avocent_6000.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murrant murrant murrant approved these changes

Assignees
No one assigned
Labels
Device 🖥️ New or added device support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@electrocret @murrant @librenms-bot @Jellyfrog