Skip to content
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

Utilities for configuring deferred restarts NRPE checks #669

Merged
merged 2 commits into from
Jan 28, 2022

Conversation

cornmetto
Copy link
Contributor

  • Added new NRPE plugin 'check_deferred_restarts.py'. Charms can use
    copy_nrpe_checks() as needed to move it to the nagios plugin directory.
  • Added {add,remove}_deferred_restarts_check utilities for adding and
    removing the NRPE checks.

* Added new NRPE plugin 'check_deferred_restarts.py'. Charms can use
  copy_nrpe_checks() to move it to the nagios plugin directory.
* Added {add,remove}_deferred_restarts_check utilities for adding and
  removing the NRPE checks.
Copy link
Collaborator

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice patch; just a few thoughts/comments for your consideration.

charmhelpers/contrib/charmsupport/nrpe.py Show resolved Hide resolved
charmhelpers/contrib/charmsupport/nrpe.py Show resolved Hide resolved
Comment on lines 19 to 20
"""
Return a list of deferred events dicts from policy-rc.d files
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's a touch picky, but please could you put the header of the docstring on the same line as the triple quotes (""")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I didn't do the same in nrpe.py because the rest of the file has them on different lines. Let me know if you think I should change those as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's fine. I'm generally in favor of just gradually improving code, but to make those changes as well would probably just confuse the purpose of this PR. Thanks for make the changes!

* nrpe.py
  * Add docstrings to the new functions.

* check_deferred_restarts.py
  * Add copyright header.
  * Improve error handling for known error conditions.
  * Improve docstrings to add raised exceptions.
Copy link
Collaborator

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks for the patch.

@ajkavanagh ajkavanagh merged commit b811ffa into juju:master Jan 28, 2022
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.

None yet

2 participants