-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
nagiosPlugins: init from pkgs/servers/monitoring/nagios/plugins #298757
nagiosPlugins: init from pkgs/servers/monitoring/nagios/plugins #298757
Conversation
af9f820
to
64be99f
Compare
64be99f
to
e208c27
Compare
@ofborg build nagiosPlugins |
375a7aa
to
75668f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this pull request and for having the courage to restructure stuff. I came across your PR while searching for existing work to update check_systemd
.
Your commit-by-commit breakdown was especially helpful in understanding your thought process. Thank you again for your hard work!
I left a few comments where I thought changes were needed, mostly related to an incorrectly converted license.
I only built check_systemd, as it was the check plugin I was looking for, and it worked as expected.
To keep things simple, I would advise against bumping packages any further, unless the upstream has been locked, as I commented.
|
||
stdenv.mkDerivation rec { | ||
pname = "check-wmiplus"; | ||
pname = "check_wmi_plus"; | ||
version = "1.65"; | ||
|
||
# We fetch from github.com instead of the proper upstream as nix-build errors | ||
# out with 406 when trying to fetch the sources | ||
src = fetchFromGitHub { | ||
owner = "speartail"; | ||
repo = "checkwmiplus"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repository is dead since October 2021, speartail/checkwmiplus@37fa4f9.
On a first glance I was unable to find another repository, but tarballs on the author's webpage: https://edcint.co.nz/checkwmiplus/releases/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkgs/servers/monitoring/nagios-plugins/check_wmi_plus/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/check_uptime/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/check_esxi_hardware/default.nix
Outdated
Show resolved
Hide resolved
3b85699
to
93dd407
Compare
Hello @oxzi I've corrected the incorrect licenses on the 3 packages. For the I suggest that we update this plugin in another PR instead. |
Thanks for your fast reply!
Merci.
I completely understand your concerns and I think your suggestion is a fine one, keep the package as it is. Perhaps you could add a comment above the |
93dd407
to
559497d
Compare
Thanks for creating the issue ticket. Sure, I just added comment above the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again! After having some hours of sleep, I looked at the changes again and they look good to me. Furthermore, I have tested building all packages and checked what happened when trying to build one of the now aliased packages. Looks good to me!
$ git rev-parse HEAD
559497dbb9ac3e5e3d7b4d7af2aca03155c559d5
$ nix-build -A nagiosPlugins
/nix/store/g7ry2rgiihhrjas9nma9gzxm2rc04r45-check_esxi_hardware-20221230
/nix/store/4xl1pkp3q6f2fj4j52ycgn45ixc7np8k-check_mssql_health-2.6.4.15
/nix/store/g4nmm8qv9q5dqgz6n0s57qdh4jsbxrkr-check_nwc_health-7.10.0.6
/nix/store/0dh1fbyl2y83wkcjgwgzx7l3513qnc2b-check_openvpn-0.0.1
/nix/store/ipksi0529nj4na81bpaxcax4akla7lis-check_smartmon-1.0.1
/nix/store/dd1gqs8fxavi6vdkyv3vg52g51b83j4c-check_ssl_cert-2.81.0
/nix/store/rrcnlcxm17c0j00m2cw703214ns0dvpd-check_systemd-4.1.0
/nix/store/kn1a4bc9sr0af2xqnc56m23hlbzxdwll-check_ups_health-2.8.3.3
/nix/store/xx6ybjx2lfvab0hmdjm5pkwcaccbkxcw-check_uptime-unstable-2016-11-12
/nix/store/b6svpmx5j9x9ww7hi34b8590nn093k4x-check_wmi_plus-1.65
/nix/store/yvg8qzjky77rg33jzjgxgaf5xn1z8pi0-check_zfs-2.0
$ nix-build -A monitoring-plugins
/nix/store/ks6afw75qpa156pv7sdkczfwx4bgydh0-monitoring-plugins-2.3.5
$ for i in check_smartmon check_systemd check_zfs check-esxi-hardware check-mssql-health check-nwc-health check-openvpn check-ups-health check-uptime check-wmiplus checkSSLCert; do nix-build -A "$i"; done
trace: warning: 'check_smartmon' has been renamed to 'nagiosPlugins.check_smartmon'
/nix/store/ipksi0529nj4na81bpaxcax4akla7lis-check_smartmon-1.0.1
trace: warning: 'check_systemd' has been renamed to 'nagiosPlugins.check_systemd'
/nix/store/rrcnlcxm17c0j00m2cw703214ns0dvpd-check_systemd-4.1.0
trace: warning: 'check_zfs' has been renamed to 'nagiosPlugins.check_zfs'
/nix/store/yvg8qzjky77rg33jzjgxgaf5xn1z8pi0-check_zfs-2.0
trace: warning: 'check-esxi-hardware' has been renamed to 'nagiosPlugins.check_esxi_hardware'
/nix/store/g7ry2rgiihhrjas9nma9gzxm2rc04r45-check_esxi_hardware-20221230
trace: warning: 'check-mssql-health' has been renamed to 'nagiosPlugins.check_mssql_health'
/nix/store/4xl1pkp3q6f2fj4j52ycgn45ixc7np8k-check_mssql_health-2.6.4.15
trace: warning: 'check-nwc-health' has been renamed to 'nagiosPlugins.check_nwc_health'
/nix/store/g4nmm8qv9q5dqgz6n0s57qdh4jsbxrkr-check_nwc_health-7.10.0.6
trace: warning: 'check-openvpn' has been renamed to 'nagiosPlugins.check_openvpn'
/nix/store/0dh1fbyl2y83wkcjgwgzx7l3513qnc2b-check_openvpn-0.0.1
trace: warning: 'check-ups-health' has been renamed to 'nagiosPlugins.check_ups_health'
/nix/store/kn1a4bc9sr0af2xqnc56m23hlbzxdwll-check_ups_health-2.8.3.3
trace: warning: 'check-uptime' has been renamed to 'nagiosPlugins.check_uptime'
/nix/store/xx6ybjx2lfvab0hmdjm5pkwcaccbkxcw-check_uptime-unstable-2016-11-12
trace: warning: 'check-wmiplus' has been renamed to 'nagiosPlugins.check_wmi_plus'
/nix/store/b6svpmx5j9x9ww7hi34b8590nn093k4x-check_wmi_plus-1.65
trace: warning: 'checkSSLCert' has been renamed to 'nagiosPlugins.check_ssl_cert'
/nix/store/dd1gqs8fxavi6vdkyv3vg52g51b83j4c-check_ssl_cert-2.81.0
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1586 |
pkgs/servers/monitoring/nagios-plugins/check_esxi_hardware/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/check_openvpn/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/check_ssl_cert/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/check_uptime/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/labs_consol_de/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/labs_consol_de/default.nix
Outdated
Show resolved
Hide resolved
pkgs/servers/monitoring/nagios-plugins/labs_consol_de/default.nix
Outdated
Show resolved
Hide resolved
7c33a68
to
20cd9a6
Compare
20cd9a6
to
6259760
Compare
@SuperSandro2000 Thank you for your review! I handled all your feedbacks. |
I was just wondering if I should rename the |
6259760
to
9012e01
Compare
Rebased against master branch and solved conflicts |
915bb4e
to
367727d
Compare
367727d
to
d5d83ea
Compare
Nice to see this getting merged. I mean.. if the original reviewer w/ merge rights did not respond, then it's easier to get merge rights yourself :) |
|
||
stdenv.mkDerivation { | ||
pname = "check-uptime"; | ||
version = "unstable-2016-11-12"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a 0- prefix
makeWrapper | ||
]; | ||
|
||
prePatch = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be postPatch
inherit pname version; | ||
|
||
src = fetchurl { | ||
url = "https://labs.consol.de/assets/downloads/nagios/${pname}-${version}.tar.gz"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pname should not be substituted in urls
@@ -231,6 +231,17 @@ mapAliases { | |||
ccloud-cli = throw "ccloud-cli has been removed, please use confluent-cli instead"; # Added 2023-06-09 | |||
certmgr-selfsigned = certmgr; # Added 2023-11-30 | |||
challenger = taler-challenger; # Added 2024-09-04 | |||
check_smartmon = throw "'check_smartmon' has been renamed to 'nagiosPlugins.check_smartmon'"; # Added 2024-05-03 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those should reference the new packages instead of using throws
As hard as it sounds, Nagios just does not have a priority in the flood of review requests I receive on a daily and it is not great that this happened like this with the 5 comments I immediately saw above. |
Thank you for the review! @SuperSandro2000 :) |
Description of changes
This pull request aims to improve the organization of Nagios plugins in our repository by creating a dedicated nagiosPlugins attribute set.
Grouping Nagios Plugins: Created a dedicated attribute nagiosPlugins in the all-packages.nix file to group all existing Nagios plugins together (check_smartmon, check_systemd, check_zfs, and check_ssl_cert)
Formatted Nagios plugins with nixfmt-rfc-style
Upgraded check_systemd to 4.1.0: Josef-Friedrich/check_systemd@v2.3.1...v4.1.0
Upgraded check_esxi_hardware to 20221230: Napsty/check_esxi_hardware@2020071...2022123
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.