Skip to content

Conversation

@ziesemer
Copy link

@ziesemer ziesemer commented Apr 4, 2023

@ziesemer
Copy link
Author

ziesemer commented Apr 4, 2023

@exhuma: I'm open to any feedback, your own updates or changes, or anything else that should be done in support towards having these updates merged. Specifically:

  1. I'm not sure if there was a better way for me to structure this into the pull request, but this includes a dependency on Initial fix for trap receiver. #111 as a prior pull request here.
  2. I experimented with a few different options around "where" to put the trap model. It didn't feel right to simply add all of it into v1.py. Adding it as a sibling to v1.py resulted in it being automatically loaded as a potential mpm plugin, which it isn't one. Ultimately, I think it closer to pdu.py - so I'm leading with including it as a sibling of that, and commenting as such within both the existing pdu.py and the new v1_trap.py.
    a. Even then, I wasn't sure how to best ensure that TrapV1 would be initialized and loaded into the X690Type type registry. For now, this is handled with a reference from v1.py.
  3. I'm still undecided as to the best types to use and return in TrapV1Content. For example, the OID's in the varbinds already came back as an ObjectIdentifier (vs. a str) - so I'm keeping the same for the enterprise. Both an ObjectIdentifier and IPv4Address add functionality on top of a base str, so it seems to make sense to keep those there - where as I can't see that a puresnmp.types.IpAddress has anything to offer over an ipaddress.IPv4Address, a x690.types.Integer over an int, or a puresnmp.types.TimeTicks over a datetime.timedelta?
  4. Additional unit test included. :-)

@ziesemer
Copy link
Author

@exhuma ?

@ziesemer
Copy link
Author

ziesemer commented Mar 8, 2024

@exhuma - is this something you can please review, consider for inclusion, and/or provide any feedback on? Thanks!

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.

1 participant