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

Add Schneider_PM55XX MIB to downloads for SNMP generator #1142

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fbegyn
Copy link
Contributor

@fbegyn fbegyn commented Mar 21, 2024

Simple addition of the Schneider MIB for the PM55XX device series.

@fbegyn fbegyn force-pushed the add-schneider-pm5xx-mib branch 2 times, most recently from d764c20 to 62be456 Compare March 21, 2024 10:40
@curl $(CURL_OPTS) -o $(TMP) $(SCHNEIDER_PM55XX_URL)
@unzip -j -d $(MIBDIR) $(TMP) PM5560_PM5563_v2.1.0/SchneiderPM55xx_V01_13.mib
# Workaround invalid character in Schneider MIB
@sed -i 's/_//g' $(MIBDIR)/SchneiderPM55xx_V01_13.mib
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still necessary, given that we support mibopts (#873) and you can simply allow those characters by adding mibopt "u" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, totally slipped my mind that is an option. Switched it out in favor of your idea. The --snmp.mibopts u works perfectly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure I'm happy with adding a MIB that requires custom mibopts. Perhaps we should adjust the generator default mibopts?

Or maybe get Schneider to fix their MIB. Cisco had some similar issues in the AIRESPACE-WIRELESS-MIB that I got them to fix. Teach them about smilint 😁

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to add an additional MIB to the downloads, it would also be nice to add an example to the generator.yml. This way we get some testing that the MIB works.

Also, this needs a rebase.

@fbegyn
Copy link
Contributor Author

fbegyn commented Jul 12, 2024

Rebased the code and tested with a few iterations locally. Still needs the --snmp.mibopts u. Any input on which way to go: have the hacky breakable sed in the Makefile or change the mibopts for the generator.

@fbegyn fbegyn requested a review from SuperQ July 12, 2024 23:06
@SuperQ
Copy link
Member

SuperQ commented Jul 13, 2024

The underscores issue seems to be somewhat common. I think we should add it to the flag default in generator/main.go.

What do you think @bastischubert?

@bastischubert
Copy link
Collaborator

I also think we should have it in the generator as it takes too much effort for contributors do deal with unclean mibs.

I missed the `--snmp.mibopts` before. By using `--snmp.mibopts u`, this error can
be prevented and it offers a much more robust way to handle the problem than
using `sed` in a makefile.

Signed-off-by: Francis Begyn <[email protected]>
Signed-off-by: Francis Begyn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants