attachment
0001-Ticket-48361-Expand-389ds-monitoring-capabilities.patch

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?

Update monitoring patch based on review.
0001-Ticket-48361-Expand-389ds-monitoring-capabilities.2.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.

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

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1692

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.