(first Bash-Script) Who wants to point out everything I did wrong?

yoYoyo · February 27, 2021, 12:05pm

Over the last few weeks, I wrote a few little scripts to automate the initial setup of a new Ubuntu(derivative) installation. Mainly automounting my Partitions and installing Programms.

As I have no expectation of competency, I can guarantee that making fun of my code will not hurt me or my ego. Thus I encourage you to take your gloves off, leave your manners at the door and enjoy yourselves.

Rather than posting the whole code here, I made a GitHub account and uploaded it there. If you’d rather that I post it here so that you can quote it more easily, just say so.

mihawk90 · February 27, 2021, 4:02pm

I haven’t gone through everything, but here’s one I noticed:

github.com

Y0-L0/pop_postinstall/blob/92b84e7d72455d7bb9aef0c8857e81eb0bc169ad/Bash-Script/automount_partitions.sh#L85


		# last Step: actually edit /etc/fstab	
		sudo echo "#" >> /etc/fstab
		# The most interesting Command in this hole script
		sudo echo "UUID=$win_uuid $win_dir ntfs umask=000,utf8 0 0" >> /etc/fstab
				
		echo		
		echo "this is the new /etc/fstab"
		cat /etc/fstab
		echo
		
		if ask "correct? "; then
			echo "$win_dir successfully mounted"
		fi
	fi
	echo "automount Complete"
fi
sudo umount -a
sudo mount -a

You ask if it’s correct, but have no way of reverting the change if it isn’t. Your user is going to have an issue and potentially broken system if it’s not, and no way to revert without knowledge what happened.

Easy solution: make a backup of fstab (sudo cp /etc/fstab /etc/fstab~) before writing to it. If the user answers that it is not correct, you can just restore the backup.

Secondly I would not umount -a (or anything for that matter), and just mount -a since you are only adding to fstab. mount -awill only mount what is not already mounted and leave the rest alone. If the user already manually mounted those partitions before running the script it will just be there on the next boot.

Also the mounting should be done before telling the user it’s done

Dutch_Master · February 27, 2021, 6:32pm

Haven’t looked at the entire code, but comment on the above snippet: the if statement is part of a function if-else so, given the above advise, add the following in the right spots:
cp /etc/fstab /etc/fstab.bak

echo "$win_dir successfully mounted"
else `cp /etc/fstab.bak /etc/fstab`
echo "changes aborted"
fi

risk · February 27, 2021, 9:14pm

See www.shellcheck.net (spotted a few unsafe patterns, if you use a programming editor/ide , try looking for a plugin).

I like putting set -eu so that my scripts stop if something fails.

Cool otherwise.