code cleanup #75
No reviewers
Labels
No labels
bug
bullseye
discussion
duplicate
enhancement
help wanted
invalid
question
suggestion
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: evolix/evocheck#75
Loading…
Reference in a new issue
No description provided.
Delete branch "cleanup"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
big cleanup
This pull-request is too big and nearly impossible to review 😞
I'll make a script to deploy the new script and compare the results with the current version, on a lot of servers. This will most probably show some regressions.
Yes indeed, and it is more than cleanup, you have rewritten some logic that could add regression.
I would suggest you to divide your work and add X merge requests.
Let's say 5 checks by 5 checks?
I already have a system in place to do the regression testing. I'll run in on all our servers.
@ -397,0 +447,4 @@
fi
else
if test -e /etc/apache2/apache2.conf; then
(grep --quiet --no-messages --extended-regexp "^env.url.*/server-status-[[:alnum:]]{4,}" /etc/munin/plugin-conf.d/munin-node \
The famous debate about long or short args.
grep -qsE
Everywhere in the script, it is short args. :)
@ -441,3 +506,3 @@
# Verification de l'activation de Squid dans le cas d'un pack mail
if [ "$IS_SQUID" = 1 ]; then
squidconffile=/etc/squid*/squid.conf
squidconffile="/etc/squid*/squid.conf"
Using quote drop the globing support!
It seems OK to me :
It fails if we quote the variable inside the
grep
command.@ -470,2 +543,3 @@
if [ "$IS_LOG2MAILRUNNING" = 1 ]; then
is_pack_web && (is_installed log2mail && pgrep log2mail >/dev/null || echo 'IS_LOG2MAILRUNNING')
if is_pack_web && is_installed log2mail; then
pgrep log2mail >/dev/null || failed 'IS_LOG2MAILRUNNING'
Don't know why it was
pgrep
here...@ -478,2 +553,3 @@
fi
is_pack_web && ( is_installed log2mail && grep -q "^file = /var/log/apache2/error.log" $conf 2>/dev/null || failed "IS_LOG2MAILAPACHE" )
if is_pack_web && is_installed log2mail; then
grep -q "^file = /var/log/apache2/error.log" $conf 2>/dev/null \
-s
in place of redirecting to /dev/null.@ -575,2 +680,3 @@
if [ "$IS_SAMBAPINPRIORITY" = 1 ]; then
is_pack_samba && grep -qrE "^[^#].*backport" /etc/apt/sources.list{,.d} && ( priority=`grep -E -A2 "^Package:.*samba" /etc/apt/preferences |grep -A1 "^Pin: release a=lenny-backports" |grep "^Pin-Priority:" |cut -f2 -d" "` && test $priority -gt 500 || failed "IS_SAMBAPINPRIORITY" )
if is_pack_samba; then
grep -qrE "^[^#].*backport" /etc/apt/sources.list{,.d} && ( priority=$(grep -E -A2 "^Package:.*samba" /etc/apt/preferences |grep -A1 "^Pin: release a=lenny-backports" |grep "^Pin-Priority:" |cut -f2 -d" ") && test $priority -gt 500 || failed "IS_SAMBAPINPRIORITY" )
This check should be using some
if; then
for better readability. As you has done with other.@ -670,1 +797,3 @@
&& grep -q '^%evolinux-sudo ALL=(ALL:ALL) ALL' /etc/sudoers.d/evolinux) || failed "IS_EVOLINUXSUDOGROUP"
(grep -q "^evolinux-sudo:" /etc/group \
&& grep -q '^%evolinux-sudo ALL=(ALL:ALL) ALL' /etc/sudoers.d/evolinux) \
|| failed "IS_EVOLINUXSUDOGROUP"
This check should be using some if; then for better readability. As you has done with other.
@ -685,2 +814,3 @@
&& test -L /etc/apache2/conf-enabled/zzz-evolinux-custom.conf \
&& test -f /etc/apache2/ipaddr_whitelist.conf) || failed "IS_APACHE2EVOLINUXCONF"
&& test -f /etc/apache2/ipaddr_whitelist.conf) \
|| failed "IS_APACHE2EVOLINUXCONF"
This check should be using some if; then for better readability. As you has done with other.
@ -701,3 +831,3 @@
if [ "$IS_BIND9MUNIN" = 1 ]; then
if is_debian_stretch && is_installed bind9; then
(test -L /etc/munin/plugins/bind9 && test -e /etc/munin/plugin-conf.d/bind9) || failed "IS_BIND9MUNIN"
(test -L /etc/munin/plugins/bind9 && test -e /etc/munin/plugin-conf.d/bind9) \
This check should be using some
if; then
for better readability. As you has done with other.@ -713,3 +844,3 @@
if [ "$IS_BROADCOMFIRMWARE" = 1 ]; then
if lspci | grep -q 'NetXtreme II'; then
(is_installed firmware-bnx2 && grep -q "^deb http://mirror.evolix.org/debian.* non-free" /etc/apt/sources.list) || failed "IS_BROADCOMFIRMWARE"
(is_installed firmware-bnx2 && grep -q "^deb http://mirror.evolix.org/debian.* non-free" /etc/apt/sources.list) \
This check should be using some
if; then
for better readability. As you has done with other.@ -722,0 +855,4 @@
fi
if lspci | grep -q 'Hewlett-Packard Company Smart Array'; then
is_installed cciss-vol-status \
|| failed "IS_HARDWARERAIDTOOL"
useless break line
@ -724,3 +862,3 @@
if [ "$IS_LOG2MAILSYSTEMDUNIT" = 1 ]; then
if is_debian_stretch; then
(systemctl -q is-active log2mail.service && test -f /etc/systemd/system/log2mail.service && ! test -f /etc/init.d/log2mail) || failed "IS_LOG2MAILSYSTEMDUNIT"
(systemctl -q is-active log2mail.service && test -f /etc/systemd/system/log2mail.service && ! test -f /etc/init.d/log2mail) \
This check should be using some
if; then
for better readability. As you has done with other.@ -729,3 +868,3 @@
if [ "$IS_LISTUPGRADE" = 1 ]; then
(test -f /etc/cron.d/listupgrade && test -x /usr/share/scripts/listupgrade.sh) || failed "IS_LISTUPGRADE"
(test -f /etc/cron.d/listupgrade && test -x /usr/share/scripts/listupgrade.sh) \
This check should be using some
if; then
for better readability. As you has done with other.@ -802,3 +947,3 @@
if [ "$IS_MARIADBSYSTEMDUNIT" = 1 ]; then
if is_debian_stretch && is_installed mariadb-server; then
(systemctl -q is-active mariadb.service && test -f /etc/systemd/system/mariadb.service.d/evolinux.conf) || failed "IS_MARIADBSYSTEMDUNIT"
(systemctl -q is-active mariadb.service && test -f /etc/systemd/system/mariadb.service.d/evolinux.conf) \
This check should be using some
if; then
for better readability. As you has done with other.@ -830,1 +974,3 @@
&& grep -q -F "command[check_mysql]=/usr/lib/nagios/plugins/check_mysql -H localhost -f ~nagios/.my.cnf") || failed "IS_MYSQLNRPE"
&& [ "$(stat -c %U ~nagios/.my.cnf)" = "nagios" ] \
&& [ "$(stat -c %a ~nagios/.my.cnf)" = "600" ] \
&& grep -q -F "command[check_mysql]=/usr/lib/nagios/plugins/check_mysql -H localhost -f ~nagios/.my.cnf") \
This check should be using some
if; then
for better readability. As you has done with other.@ -835,3 +982,3 @@
if is_debian_stretch && is_installed php; then
(test -f /etc/php/7.0/cli/conf.d/z-evolinux-defaults.ini \
&& test -f /etc/php/7.0/cli/conf.d/zzz-evolinux-custom.ini) || failed "IS_PHPEVOLINUXCONF"
&& test -f /etc/php/7.0/cli/conf.d/zzz-evolinux-custom.ini) \
This check should be using some
if; then
for better readability. As you has done with other.@benpro I need explanation for this portion of code, from the current master branch :
I don't really understand what situation must trigger the
failed
call.I think it is:
If it is not a stretch we check that server-status with 4 digits is properly configured in
/etc/munin/plugin-conf.d/munin-node
and/etc/apache2/apache2.conf
or/etc/apache2/mods-enabled/status.conf
@benpro the current tests were failing when the server-status was defined in another file in
/etc/munin/plugin-conf.d/
hence the weird failures in the master branch. And we didn't see it probably because there was a logic error in the nested parenthesis.I've fixed this in 67765a03c4 and d814936446
I've done a ton of small fixes.
Now even shellcheck is happy (with 5 or 6 specific style warnings disabled).
I've added an option parser, just like evomaintenance.
@ -373,1 +502,3 @@
test -d /etc/nagios && ls -ld /etc/nagios | grep -q drwxr-x--- || failed "IS_NRPEPERMS"
if [ -d /etc/nagios ]; then
actual=$(stat --format "%A" /etc/nagios)
expected="drwxr-x---"
Check with numbers.
@ -397,0 +543,4 @@
&& test -h /etc/munin/plugins/apache_accesses \
&& test -h /etc/munin/plugins/apache_processes \
&& test -h /etc/munin/plugins/apache_volume; } \
|| failed "IS_APACHEMUNIN" "mising munin plugins for Apache"
missing
@ -493,2 +678,2 @@
if [ "$(md5sum /usr/sbin/named |cut -f 1 -d ' ')" != "$(md5sum /var/chroot-bind/usr/sbin/named |cut -f 1 -d ' ')" ]; then
failed "IS_BINDCHROOT"
if is_installed bind9; then
if netstat -utpln | grep "/named" | grep :53 | grep -qvE "(127.0.0.1|::1)"; then
ss inplace of netstat
using
lsof -i :53
in place ofnetstat…
.@ -577,0 +793,4 @@
priority=$(grep -E -A2 "^Package:.*samba" /etc/apt/preferences | grep -A1 "^Pin: release a=.*-backports" | grep "^Pin-Priority:" | cut -f2 -d" ")
test "$priority" -gt 500 || failed "IS_SAMBAPINPRIORITY"
fi
fi
Limited to Lenny.
@ -677,1 +947,3 @@
groups $user |grep -q adm || failed "IS_USERINADMGROUP"
users=$(grep "^evolinux-sudo:" /etc/group | awk -F: '{print $4}' | tr ',' ' ')
for user in $users; do
groups "$user" | grep -q adm || failed "IS_USERINADMGROUP" "User $user doesn't belong to \`adm' group"
Maybe add a break.
@ -1061,2 +1380,3 @@
if [ "$IS_TMP_1777" = 1 ]; then
ls -ld /tmp | grep -q drwxrwxrwt || failed "IS_TMP_1777"
actual=$(stat --format "%A" /tmp)
expected="drwxrwxrwt"
Numbers.
@ -1065,2 +1386,3 @@
if [ "$IS_ROOT_0700" = 1 ]; then
ls -ld /root | grep -q drwx------ || failed "IS_ROOT_0700"
actual=$(stat --format "%A" /root)
expected="drwx------"
Numbers
@ -1069,2 +1392,3 @@
if [ "$IS_USRSHARESCRIPTS" = 1 ]; then
ls -ld /usr/share/scripts | grep -q drwx------ || failed "IS_USRSHARESCRIPTS"
actual=$(stat --format "%A" /usr/share/scripts)
expected="drwx------"
Numbers
@benpro I've commmited the changes I was comfortable doing (ie. all of them but the "netstat/ss" replacement and "lsof").
As far as I cal tell, it's good to merge.
WIP: code cleanupto code cleanupthere are conflicts, but I guess it's going to be mergeable when #70 is merged, or i'll deal with the remaining conflicts