-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix: collapsed fields by calling more indepth function #10430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, no test changes and all tests succeed, so no change in behaviour? Do you have before and after output please?
I did not test against the user's mib. Just implemented the idea you were adamant about. |
Adamant? Sorry, if you have another or better solution, please be my guest and implement as You want. I just want to make sure the previous behaviour is restored.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good that you found a way without using smi.RenderOid
, but I think there are still some mistakes.
I don't think this is the right way to do either. I am going to mark this as a draft and try a different idea. |
The latest build even hangs when trying to start up.. Nevermind, that was my fault by trying to put it in a docker container 😛 |
@reimda and I took a look at the failing tests and noticed that the test2 mibs file hadn't setup sysUpTimeInstance and coldstart correctly therefore it was adding the trailing numbers we went ahead and fixed it and pushed a commit. The new code for |
Yes I know it needs to have a test. I asked @MyaLongmire to help with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MyaLongmire, please make the error messages starting lowercase... :-)
@@ -111,22 +111,29 @@ type MibEntry struct { | |||
} | |||
|
|||
func TrapLookup(oid string) (e MibEntry, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be nicer if we just passed an Oid
type instead of string?
func TrapLookup(oid string) (e MibEntry, err error) { | |
func TrapLookup(givenOid types.Oid) (e MibEntry, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The oid arrives from the trap through gosnmp as a string. If we make TrapLookup take gosmi's types.Oid, we would have to convert from string to types.Oid everywhere TrapLookup (aka lookupFunc) is called. Makes sense to me to leave the argument a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. I don't have any preference regarding @Hipska's comment on passing an OID struct instead of a string. However, the commented code section should be removed.
… to internal/snmp. Remove remaining translate test from plugins/inputs/snmp_trap.
📦 Looks like new artifacts were built from this PR. Expand this list to get them here ! 🐯Artifact URLs |
(cherry picked from commit 84c1efb)
Required for all PRs:
resolves #9470
Changes calling
node.RenderQualified
to callingsmi.RenderOid
as a suggestion of @Hipska