search

Chatnaut / Arclight

An open source server virtualization management solution based on QEMU/KVM. Manage virtual machines, containers, highly available clusters, storage and networks with an integrated, easy-to-use web interface or via CLI. (Featured on zeupiter.com)
https://chatnaut.com
Other
158 stars 13 forks source link

Please lint your shell scripts! #16

Closed desrod closed 1 year ago

desrod commented 1 year ago

It's a simple matter to clean and lint your shell scripts with tools like shellcheck and others.

There are some pretty odd constructs in your scripts which could use some additional attention, especially since they're intended to be run as root.

For example: 

if [ $(egrep -c '(vmx|svm)' /proc/cpuinfo) -eq 0 ]; then

Why not just use kvm-ok, like other modern architectures are using. Simply having the CPU flags exposed doesn't guarantee they can be used for successful virtualization.

Another odd one I found: 

apt install php-dev php-pear -y
apt-get install mongodb
sudo apt install -y php-dev

Why are these 3 separate commands, and why three separate ways of executing them? Why is php-dev being installed twice?

How about just: 

sudo apt -y install php-dev php-pear mongodb

This is done in a couple of places. Let's fix that first.

Also, why these two: 

tar -xzf v2.0.0.tar.gz
mv Arclight-2.0.0 arclight

When you can just do: 

tar -xzf v2.0.0.tar.gz -C arclight

You're also not doing any error checking on these commands, if they fail, or if commands you expect to be there are not (like a2enmod).

And using service has been deprecated long ago. You want systemctl, so you can use the more modern systemd way of managing services.

This, is also bad, and overwrites the user's intended Python interpreter, without their knowledge: 

ln -s /usr/bin/python3 /usr/bin/python

Don't do that. Use the proper Debian facilities, dpkg-divert or dpkg-reconfigure python3.

It's a great start for a v0.0 of your project, but let's keep it clean, POSIX correct, and secure enough that it won't break an existing user's system without lots of untangling.

elondust commented 1 year ago

@desrod Thanks a lot for raising this issue, All your point are noted and will be implemented in the codebase.

desrod commented 1 year ago

As a matter of process, you really shouldn't close opened, acknowledged issues, without a commit that addresses those issues.

This issue remains open, and I don't see any commits referencing these fixes, so why is the issue being closed?