Number of additions and improvements

[SimplicityGuy]

Updated a number of things:

• Added ability to set an OID by name
• Added timeout and retries to object initialization
• Added new test, for use with an APC-8xxx PDU
• Fixed compatibility issues with pysnmp 4.5.2
• PEP8 formatting improvements

[seveas]

Thanks!
I'll do a more thorough review later, but I had a quick look and most changes seem ok. I do wonder why you have split up authorizations in read and write authorizations. This is not backwards compatible and I don't see why it's needed, so I must be missing something 😄

[SimplicityGuy]

Thanks for the initial review!

Usually the write authorizations are different than the read authorizations. I think understand your point... Since the authorizations is a list, I could append the write authorization to the list.

My concern with this approach is that without actually writing to an oid you don't really know whether the authorization is for reading or writing. So with it broken out into a separate parameter the caller can explicitly make the call which authorization is for read vs writing.

Also, the changes should be compatible, however it looks like I failed to do that. I'll add a default (None) for the write authorization parameter so that it is compatible.

[seveas]

You're saying you have authorizations that can write but cannot read? In that case I'd use two snmpclient objects instead of two lists of authorizations.

[seveas]

Though looking at that code a bit further. The whole authorization selection in my original code is a bit bonkers. One should just give it a valid authorization instead of making snmpclient try more than one until one works. That's a backwards compatibility breakage I can accept 😄

[SimplicityGuy]

Correct. Most SNMP devices use two different logins for the operation, usually private for writing, and public for reading.

[SimplicityGuy]

I'm happy to re-write the authorizations to just take a dictionary with a single entry for reading, and another entry for writing. Reading entry is required, writing entry is optional.

Does that sound good?

[seveas]

I've not seen that before. In my environment, the private one can always be used for both reading and writing. Let me ponder a bit about what I want to do here.

[SimplicityGuy]

Interesting. All the APC products ship with a default of public/private community strings with the distinction of which one is read-only vs read/write.

Since some oid are read/write, I would argue that reads should happen with the read only account, and writes with the other, applying the principal of least privilege needed for an operation.

In any case, let me know how you want to change it; I'm happy to make the change and flexible with the signature of init.

[seveas]

I have a different interpretation of that principle. I wouldn't separate per function call, but per script/tool. If a script doesn't need r/w access, it gets the r/o user. If it needs write access, it gets the r/w user and uses it for both reading and writing.

So I'm probably going to rip out both my old crappy auth selection (which was there for a reason that has gone away) and the separate read and write auths, and just require users to provide a single authorization.

Apart from that I have the following comments:

• I'll skip the AUTHORS file and add a COPYING file with the same information and a license. Are you OK with the zlib license (it's currently GPL)?
• pep8's line length rule belongs in the 90's. Readability is what counts, not artificial line length limits.
• I'll merge and reorder some patches so there are no fixup patches
• Why did you move the call to load_mibs, that seems unnecessary?
• The big if/else chain in set seems unnecessary. Wouldn't this suffice: value = type(initial_value)(str(value))?
• Why is 372b88c necessary? That would be good to have in the commit message.

[seveas]

One more comment: what's your minimum python version requirement? I'll port the library to python 3 while I'm at it and want to kill support for python 2.6 and older if possible.

[SimplicityGuy]

• Minimum Python version is 2.7 for me. Likely move to Python 3.x sometime later this year.
• zlib license seems fine
• hehe, PEP8 is always a point of contention. Generally I try to follow PEP8 with 80 col limit for those wanting multiple vim or emacs terminals side-by-side. Personally I think it improves readability too.
• load_mibs was moved primarily since this seemed out of place between the original two calls. Not certain the new spot is any better though.
• 372b88c... good point. I should have left a better commit comment at the time. IIRC I hit an issue where it didn't work without it, compatibility to the updated pysnmp. However reviewing the docs for this seems like this is probably unnecessary.

I'm fine to rip out the old auth mechanism and go with usage being that one object is used for reading and one for writing, each with different authorizations.

How do you want to proceed? Do you want me to make the changes? Or will you merge and make the changes?

[seveas]

I'm going to do some preparatory commits (I'm not really happy with the current state of the project) and cherry-pick the parts of your commits I want to keep. Then I'll push that out on a branch for discussion. Stay tuned!

[SimplicityGuy]

Ok, let me know if you need to do jump in.

[SimplicityGuy]

Regarding the big if clause, yep you're right! That's a lot cleaner.

[SimplicityGuy]

Any update on this merge or the other rework?