#48361 Expand 389ds monitoring capabilities
Closed: Fixed None Opened 3 years ago by firstyear.

This patch expands the 389ds monitoring functions to help improve the quality of tools we can write.


Hi William,

thank you for expanding lib389 tools capabilities. :)

There is a few issues with your patch:
* I can't apply your patch on the fresh updated repository, the line numbers are incorrect. Please, check that you've updated your local repo with "git pull" after Mark's big patch.

  • I guess, there should be DN_MONITOR_SNMP instead of DN_MONITOR:

{{{
35 snmp_status = self.conn.search_s(DN_MONITOR,
ldap.SCOPE_BASE, '(objectClass=*)')
}}}

  • The last but not least. Can you please expand your commit message?
    Just try to answer on this three questions:

    1. Why is this change necessary?
    2. How does it address the issue?
    3. What side effects does this change have?

This three basic questions are common for any commit message. Some of them may be optional in some situations, but try stick to them, it will increase an understanding of your actions.
Also, consider making including a link to the issue. :)

And a few more things that need to be clarified:
* Do we really need this line? I guess, if we return value, it is already enough.

{{{
43 print(status)
}}}

  • You create object '''snmp_status''' and don't use it. Why?
  • I can't apply your patch on the fresh updated repository, the line numbers are incorrect. Please, check that you've updated your local repo with "git pull" after Mark's big patch.

Fixed!

  • I guess, there should be DN_MONITOR_SNMP instead of DN_MONITOR:

{{{
35 snmp_status = self.conn.search_s(DN_MONITOR,
ldap.SCOPE_BASE, '(objectClass=*)')
}}}

Yep, probably because I wasn't using it yet, I didn't notice the mistake.

  • The last but not least. Can you please expand your commit message?
    Just try to answer on this three questions:

    1. Why is this change necessary?
    2. How does it address the issue?
    3. What side effects does this change have?

This three basic questions are common for any commit message. Some of them may be optional in some situations, but try stick to them, it will increase an understanding of your actions.
Also, consider making including a link to the issue. :)

Fixed, I expanded on this a bit and why we are doing it.

And a few more things that need to be clarified:
* Do we really need this line? I guess, if we return value, it is already enough.

{{{
43 print(status)
}}}

Missed removing this before I committed. Removed now.

  • You create object '''snmp_status''' and don't use it. Why?

Because I plan to use it in the future but not yet: For now I have commented this out until I need it.

I am going to expand this and other lib389 functions incrementally, and over time as I need to use them and consume them in rest389, and I'd rather do it in small steps where we are properly consuming all of the data we are retrieving rather than making lib389 do a whole bunch, and only use a subset of the capabilities we add.

Thanks for always taking the time to review these carefully for me.

Thank you! It is great!

Ack.

P.S. can you please move link to the issue from commit subject to the commit body? This is much better and readable to have commit subject limited by 50 characters. :) Also, it is part of our workflow(to have a link to the issue at the bottom of the commit body). Sorry for my tediousness.

commit 252e0c17e259e10d60e2f0b572ee64ae0940fb10
117a8ab..252e0c1 master -> master

Milestone lib389 1.0 deleted

Metadata Update from @mreynolds:
- Issue assigned to firstyear

2 years ago

Login to comment on this ticket.

Metadata