Skip to content

Conversation

@voetberg
Copy link
Contributor

Address part of #127 - Update without Prometheus

Copy link
Contributor

@dchristidis dchristidis left a comment

Choose a reason for hiding this comment

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

Please split your PR into multiple commits. You can take #138 as an example.

It may seem tedious for such a small and simple probe, but it’s a good habit, one that should be adopted early.

@voetberg voetberg force-pushed the 127-modern-expired-locked-rules branch from 92ff4c5 to 8899df8 Compare August 29, 2025 14:37
Copy link
Contributor

@dchristidis dchristidis left a comment

Choose a reason for hiding this comment

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

You’re also porting the probe from Python 2 to Python 3, but that is not described in your commits. The changes to the shebang and the print statements should be split in a separate commit.

In case you’re not convinced by the usefulness of this, there was a print 'Locked expired rules' statement that you silently removed. It required effort from me to notice it; it would have been obvious if the changes had been split as recommended.

status = CRITICAL
print row[0], row[1], row[2]
print(row[0], row[1], row[2])

Copy link
Contributor

Choose a reason for hiding this comment

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

This blank line (and the similar one you introduced later) produces a curious grouping. According to PEP 8:

Use blank lines in functions, sparingly, to indicate logical sections.

A try and its associated except statements are typically part of the same section. It would make more sense to introduce a blank line before the second try statement and before the last sys.exit(status) call.

@voetberg voetberg force-pushed the 127-modern-expired-locked-rules branch from 8899df8 to ec4b89b Compare September 23, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants