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.

Sign up for GitHub

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

Jump to bottom

check_systemd: Fix executable name and include latest arguments #10035

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

check_systemd: Fix executable name and include latest arguments #10035

wants to merge 2 commits into from

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Apr 3, 2024

The executable name for check_systemd's dropped the .py suffix for version 2.0.31, released in April 20192. However, the old name is still being referenced, both in documentation as well as in the ITL's CheckCommand's command, making it unusable.

Closes #9547.

Footnotes

  1. https://github.com/Josef-Friedrich/check_systemd/compare/v2.0.2...v2.0.3#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7 ↩

  2. https://github.com/Josef-Friedrich/check_systemd/releases/tag/v2.0.3 ↩

@oxzi oxzi added area/documentation End-user or developer help area/itl Template Library CheckCommands labels Apr 3, 2024
@cla-bot cla-bot bot added the cla/signed label Apr 3, 2024
@icinga-probot icinga-probot bot added bug Something isn't working good first issue Good for newcomers labels Apr 3, 2024
yhabteab
yhabteab reviewed May 7, 2024
View reviewed changes
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

Can you please rebase the PR to get the GAs fixed and apart from that, you may want to consider adding this one missing option as well.

-i, --ignore-inactive-state
                        Ignore an inactive state on a specific unit. Oneshot
                        services for example are only active while running and
                        not enabled. The rest of the time they are inactive.
                        This option has only an affect if it is used with the

@yhabteab yhabteab added this to the 2.15.0 milestone May 7, 2024
@oxzi
check_systemd: Fix executable name by dropping .py
a219436 
The executable name for check_systemd's dropped the `.py` suffix for
version 2.0.3[0], released in April 2019[1]. However, the old name is
still being referenced, both in documentation as well as in the ITL's
CheckCommand's command, making it unusable.

Closes #9547.

[0]: Josef-Friedrich/check_systemd@v2.0.2...v2.0.3#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7
[1]: https://github.com/Josef-Friedrich/check_systemd/releases/tag/v2.0.3
oxzi added a commit to oxzi/check_systemd that referenced this pull request May 8, 2024
@oxzi
contrib/icinga2: harmonize Icinga 2 config w/ ITL
8d21d01 
While fixing the outdated command name in the Icinga 2 ITL[0], it was
suggested to support the newer command line options there as well.

To synchronize or harmonize the ITL definition and the CheckCommand
shipped here, I made small changes to not break the ITL, as I would
guess that more people are using the ITL definition instead of the
shipped command.conf from here.

The breaking changes were added to the CHANGELOG.

[0]: Icinga/icinga2#10035
@oxzi @RincewindsHat
check_systemd: harmonize ITL w/ upstream
104ef2c 
Harmonize the arguments with the upstream CheckCommand[0], including a
patch to use the ITL variables[1].

[0]: https://github.com/Josef-Friedrich/check_systemd/blob/main/contrib/icinga2/command.conf
[1]: Josef-Friedrich/check_systemd#38

Co-Authored-By: RincewindsHat <12514511+RincewindsHat@users.noreply.github.com>
@oxzi
Copy link
Member Author

oxzi commented May 8, 2024 •
edited

Rebased.

I have also harmonized the arguments with the upstream CheckCommand, including a patch to use the ITL variables.

As @RincewindsHat initially created the CheckCommand on the upstream repository, I have added him as a co author. Due to the slightly different licenses, I would kindly ask him to approve this PR.

Edit: My patch was already merged upstream. They are fast!

@oxzi oxzi changed the title check_systemd: Fix executable name by dropping .py check_systemd: Fix executable name and include latest arguments May 8, 2024
@yhabteab
Copy link
Member

yhabteab commented May 8, 2024

I have also harmonized the arguments with the upstream CheckCommand, including a patch to use the ITL variables.

Initially I just wanted you to add the one missing argument --ignore-inactive-state, being the only argument that couldn't be set anyhow. But now you've added all the arguments which's even better, but most importantly you've renamed some existing custom vars and introduced a breaking change in Icinga 2 as well, e.g. configurations that previously set vars.systemd_exclude_unit with wildcard matches will no longer work. Also, the use of {{{...}}} for the description in the itl is quite uncommon and the texts are always reduced to the bare essentials.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhabteab yhabteab yhabteab left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/documentation End-user or developer help area/itl Template Library CheckCommands bug Something isn't working cla/signed good first issue Good for newcomers
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

Wrong executable name for check_systemd plugin
2 participants
@oxzi @yhabteab