complexorganizations/wireguard-manager

Code refactoring

Rebelllious opened this issue · 9 comments

Rebelllious commented

I have had no possibility to put this into a pull request, but here are some of my ideas.

  1. It is easier to maintain a list of supported distros in an array-like form including breakdown into distinguished families that usually share the same software or commands run in the script:
SUPPORTED_DISTROS=("ubuntu" "debian" "raspbian" "pop" "kali" "linuxmint" "neon"
           "fedora" "centos" "rhel" "almalinux" "rocky" "ol"
           "arch" "archarm" "manjaro"
           "alpine"
           "freebsd")

DEBIAN_LIKE=("ubuntu" "debian" "raspbian" "pop" "kali" "linuxmint" "neon")
RHEL_LIKE=("fedora" "centos" "rhel" "almalinux" "rocky" "ol")
ARCH_LIKE=("arch" "archarm" "manjaro")
  1. Most packages installed as part of installing-system-requirements() function have the same name across all the supported distros, with only some differences:
# identical package names for all supported distros
SOFTWARE_DEFAULT=("curl" "coreutils" "jq" "lsof" "gawk" "grep" "qrencode" "sed" "zip" "unzip" "openssl" "nftables" "e2fsprogs" "gnupg" "systemd")
# For DEBIAN_LIKE
SOFTWARE_DEBIAN=("iproute2" "cron" "procps" "ifupdown")
# For RHEL_LIKE
SOFTWARE_RHEL=("iproute" "cronie" "procps-ng" "NetworkManager")
# For ARCH_LIKE
SOFTWARE_ARCH=("iproute2" "cronie" "procps-ng" "ifupdown")
# Alpine and FreeBSD have identical package names, based on the current code
SOFTWARE_OTHER=("iproute2" "cronie" "procps" "ifupdown")
  1. the installing-system-requirements() function can then be transformed to a way more readable and supportable state as below:
function installing-system-requirements() {
  if [ ${SUPPORTED_DISTROS[*]} =~ "${CURRENT_DISTRO}" ]; then
    if [ ${DEBIAN_LIKE[*]} =~ "${CURRENT_DISTRO}" ]; then
      apt-get update
      apt-get ${SOFTWARE_DEFAULT[@]} ${SOFTWARE_DEBIAN[@]} -y
    elif [ ${RHEL_LIKE[*]} =~ "${CURRENT_DISTRO}" ]; then
      yum check-update
      yum install epel-release elrepo-release -y
      yum install ${SOFTWARE_DEFAULT[@]} ${SOFTWARE_RHEL[@]} -y
    elif [ ${ARCH_LIKE[*]} =~ "${CURRENT_DISTRO}" ]; then
      pacman -Sy
      pacman -S --noconfirm --needed ${SOFTWARE_DEFAULT[@]} ${SOFTWARE_ARCH[@]}
    elif [ "${CURRENT_DISTRO}" == "alpine" ] ; then
      apk update
      apk ${SOFTWARE_DEFAULT[@]} ${SOFTWARE_OTHER[@]}
    elif [ "${CURRENT_DISTRO}" == "freebsd" ]; then
      pkg update
      pkg ${SOFTWARE_DEFAULT[@]} ${SOFTWARE_OTHER[@]}
    fi
  else
    echo "Error: ${CURRENT_DISTRO} ${CURRENT_DISTRO_VERSION} is not supported."
    exit
  fi
}

I suggest removing the check for the executables that is in the parent if statement of the function as I truly don't understand why it is there. We simply instruct the script to install the required packages - no problem if they are already installed. In the majority of cases those are missing anyway and the gain of having that check there is doubtful, at least as I see it.

Deleted user commented

Hey,
I agree with u on everything except a single thing regarding the application check, let's say we install curl and then the script runs again, it will try to install curl again.

You might ask, why run the requirement check if it's already installed?
Because people are stupid, and they will remove the requirements, I have seen people remove curl and, grep, sed and other tools.

Also, one more thing, if u remove that line from the file, it will try to install it every single time you run the script, wasting your time even more.

Rebelllious commented

It will not try installing curl in such a case as the package is already installed. It will tell you "curl installed and already the latest version"

Deleted user commented

It will not try installing curl in such a case as the package is already installed. It will tell you "curl installed and already the latest version"

yes, that takes about 3 seconds on average to get to, VS if u have that check in there you do not have to try and install it again.

Deleted user commented

check.sh

if [ ! -x "$(command -v curl)" ]; then
    apt-get install curl -y
    echo "Done"
fi

no-check.sh

apt-get install curl -y
echo "Done"
root ➜ /workspaces/wireguard-manager (main ✗) $ time bash no-check.sh 
Reading package lists... Done
Building dependency tree       
Reading state information... Done
curl is already the newest version (7.68.0-1ubuntu2.7).
0 upgraded, 0 newly installed, 0 to remove and 4 not upgraded.
Done

real    0m1.145s
user    0m1.090s
sys     0m0.057s
root ➜ /workspaces/wireguard-manager (main ✗) $ time bash check.sh 

real    0m0.003s
user    0m0.003s
sys     0m0.000s
root ➜ /workspaces/wireguard-manager (main ✗) $
Rebelllious commented

Well, if I were to choose between 3 seconds delay at deployment vs cleaner code easier to support I would choose the latter. But that's my own opinion, of course. Also, it's now trying to install it anyway - if one of those packages there is missing, it runs installation for the whole list.

Deleted user commented

Testing this on github codespaces.

This has the check on it.

root ➜ /workspaces/wireguard-manager (main ✗) $ time bash wireguard-manager.sh --ddns
What do you want to do?
   1) Show WireGuard
   2) Start WireGuard
   3) Stop WireGuard
   4) Restart WireGuard
   5) Add WireGuard Peer (client)
   6) Remove WireGuard Peer (client)
   7) Reinstall WireGuard
   8) Uninstall WireGuard
   9) Update this script
   10) Backup WireGuard
   11) Restore WireGuard
   12) Update Interface IP
   13) Update Interface Port
   14) Purge WireGuard Peers
   15) Generate QR Code

real    0m2.213s
user    0m0.086s
sys     0m0.015s

Removed the check on this one.

root ➜ /workspaces/wireguard-manager (main ✗) $ time bash wireguard-manager.sh --ddns
Hit:1 https://repo.anaconda.com/pkgs/misc/debrepo/conda stable InRelease
Hit:2 http://security.ubuntu.com/ubuntu focal-security InRelease                                        
Hit:4 https://cli.github.com/packages focal InRelease                                                   
Err:5 https://packages.microsoft.com/repos/azure-cli focal InRelease    
  Temporary failure resolving 'packages.microsoft.com'
Hit:6 http://ppa.launchpad.net/xapienz/curl34/ubuntu focal InRelease                                                                                        
Hit:7 http://archive.ubuntu.com/ubuntu focal InRelease                                                                                                      
Hit:8 http://archive.ubuntu.com/ubuntu focal-updates InRelease                                                            
Hit:9 http://archive.ubuntu.com/ubuntu focal-backports InRelease                                             
Hit:10 https://packages.microsoft.com/repos/microsoft-ubuntu-focal-prod focal InRelease                      
Err:3 https://packagecloud.io/github/git-lfs/ubuntu focal InRelease                    
  Temporary failure resolving 'd28dx6y1hfq314.cloudfront.net'
Reading package lists... Done                   
W: Failed to fetch https://packages.microsoft.com/repos/azure-cli/dists/focal/InRelease  Temporary failure resolving 'packages.microsoft.com'
W: Failed to fetch https://packagecloud.io/github/git-lfs/ubuntu/dists/focal/InRelease  Temporary failure resolving 'd28dx6y1hfq314.cloudfront.net'
W: Some index files failed to download. They have been ignored, or old ones used instead.
Reading package lists... Done
Building dependency tree       
Reading state information... Done
coreutils is already the newest version (8.30-3ubuntu2).
cron is already the newest version (3.0pl1-136ubuntu1).
e2fsprogs is already the newest version (1.45.5-2ubuntu1).
gawk is already the newest version (1:5.0.1+dfsg-1).
grep is already the newest version (3.4-1).
iproute2 is already the newest version (5.5.0-1ubuntu1).
sed is already the newest version (4.7-1).
unzip is already the newest version (6.0-25ubuntu1).
zip is already the newest version (3.0-11build1).
ifupdown is already the newest version (0.8.35ubuntu1).
nftables is already the newest version (0.9.3-2).
qrencode is already the newest version (4.0.2-2).
curl is already the newest version (7.68.0-1ubuntu2.7).
gnupg is already the newest version (2.2.19-3ubuntu2.1).
lsof is already the newest version (4.93.2+dfsg-1ubuntu0.20.04.1).
openssl is already the newest version (1.1.1f-1ubuntu2.12).
openssl set to manually installed.
procps is already the newest version (2:3.3.16-1ubuntu2.3).
systemd is already the newest version (245.4-4ubuntu3.15).
jq is already the newest version (1.6-1ubuntu0.20.04.1).
0 upgraded, 0 newly installed, 0 to remove and 48 not upgraded.
1 not fully installed or removed.
After this operation, 0 B of additional disk space will be used.
Setting up resolvconf (1.82) ...
resolvconf.postinst: Error: Cannot replace the current /etc/resolv.conf with a symbolic link because it is immutable; to correct this problem, gain root privileges in a terminal and run 'chattr -i /etc/resolv.conf' and then 'dpkg --configure resolvconf'; aborting
dpkg: error processing package resolvconf (--configure):
 installed resolvconf package post-installation script subprocess returned error exit status 1
Errors were encountered while processing:
 resolvconf
E: Sub-process /usr/bin/dpkg returned an error code (1)
What do you want to do?
   1) Show WireGuard
   2) Start WireGuard
   3) Stop WireGuard
   4) Restart WireGuard
   5) Add WireGuard Peer (client)
   6) Remove WireGuard Peer (client)
   7) Reinstall WireGuard
   8) Uninstall WireGuard
   9) Update this script
   10) Backup WireGuard
   11) Restore WireGuard
   12) Update Interface IP
   13) Update Interface Port
   14) Purge WireGuard Peers
   15) Generate QR Code

real    0m48.491s
user    0m2.850s
sys     0m0.422s
root ➜ /workspaces/wireguard-manager (main ✗) $
Rebelllious commented

Why do you need to run WireGuard installation script on a server that has already got it - and this all packages - installed? Correct me if I am wrong, but the only workflow involving checking for those packages is installation, nothing else. I assume the amount of those that have already got all those packages installed is really small for the initial install baseline (and could thus be disregarded unless causing pain in the back later), and no normal/sane person would run the full installation on one's server for the second time

Rebelllious commented

@Prajwal-Koirala to be honest, I have had no exposure to drones so far. I will check what it's all about.
And what's the ask there?

Prajwal-Koirala commented

I'm ready to provide information and insights based on the GitHub issue you've presented.

Here's a summary of key points:

Issue:

  • Discussion about refactoring code for better readability and maintainability.
  • Specific focus on the installing-system-requirements() function.

Key Suggestions:

  • Use arrays to group supported distros and packages for easier management.
  • Remove unnecessary checks for executables to streamline installation.
  • Consider timing benchmarks to evaluate performance impacts of changes.

Disagreements:

  • Whether to include checks for existing packages:
    • Rebelllious argues for removing them, prioritizing code clarity.
    • Ghost advocates for keeping them to avoid unnecessary installations.

Additional Considerations:

  • Alpine Linux compatibility challenges due to cut command and systemd differences.
  • Potential strategies to address these challenges:
    • Investigate cut version compatibility.
    • Explore alternative text manipulation tools.
    • Adapt script logic to Alpine's OpenRC init system.
    • Consider alternative service management tools.
    • Contribute patches or workarounds to enhance Alpine compatibility.
    • Explore other WireGuard management tools with native Alpine support.

Next Steps:

  • Developers to consider feedback and make decisions about code changes.
  • Potential for further testing and discussion to refine refactoring approach.

I'm here to assist further if you have specific questions or require additional information.