Bash script project - Need people to contribute

[Tristan_Williams]

I need people to help me improve a bash script.

I am creating a bash script that will serve as an automated system maintenance and backup tool, mainly for Ubuntu beginners.

The script can perform backup of the /home directory, maintain packages through apt-get, and maintain PPAs.

Here is the first version of the script:

#!/bin/bash

# This is the nls-maintain script
# This is used on NLSLinux systems for basic system maintenance.
# This was developed by Tristan Williams, Owner of Newlife Linux Solutions
# This script is free and open source software
# Feel free to change it any way you like



function nls_main {

    read -p "Confirm you want to run maintenance [y/n] " nls_conf

        if [ "$nls_conf" == "y" ]; then
            unset nls_conf
            nls_maintain
        elif [ "$nls_conf" == "n" ]; then
            unset nls_conf
            exit
        else
            unset nls_conf
            nls_err_msg
        fi
}

# Function for system maintenance
function nls_maintain {

    clear
    
    # Ask user if they want to do a system backup before maintenance
    read -p "Perform system backup before maintenance? [y/n] " nls_bkup_yn
        if [ "$nls_bkup_yn" == "y" ]; then
            # Determine drive to hold backup
            read -p "Backup to which drive? (example: sdb1)" nls_bkup_drive
            
            # Mount backup drive
            mount -v $nls_bkup_drive /mnt/
            
            # Remove previous backup, so that old files aren't kept
            sudo rm -rf /mnt/nls_bkup/
            
            # Re-make backup directory on backup drive
            sudo mkdir /mnt/nls_bkup/
            
            # Copy home folders to backup drive
            sudo cp -Rv /home/ /mnt/nls_bkup/
            
            # Unmount backup drive to avoid issues with file managers later
            sudo umount -v $nls_bkup_drive
            
            # Inform user of backup location
            echo "Backups are stored in the drive which you specified, in a directory named nls_bkup"
            echo ""
            read -p "Press any key to start maintenance"
            
            # Unset old variables
            unset nls_bkup_drive
            unset nls_bkup_yn
            
            nls_no_bkup_maintain
        elif [ "$nls_bkup_yn" == "n" ]; then
        
            # Unset old variables
            unset nls_bkup_drive
            unset nls_bkup_yn
            
            nls_no_bkup_maintain
        else
            unset nls_bkup_yn
            nls_loc="nls_maintain"
            nls_err_msg
        fi
    
    # Maintenance
    function nls_no_bkup_maintain {
    
        clear
        
        # Determine if user wants to check for broken PPAs
        read -p "Check for broken PPAs? [y/n] " nls_maintain_ppa_check
            # Check for broken PPAs
            if [ "$nls_maintain_ppa_check" == "y" ]; then
                
                clear
                echo "Checking for broken PPAs... "
                sudo apt-get update > update_app.txt
                clear
                cat update_app.txt | grep Err | cut -c 12- > broken_app.txt 

                IFS=$'\n' read -d '' -r -a lines < broken_app.txt
                 
                 # List broken PPA(s)
                echo "Found the following broken ppa's"
                echo "${lines[@]}"
                echo ""
                echo "If there are no PPAs listed above, reply 'n'"
                echo "to the prompt below"
                echo ""
                echo "It may be that the repositories are down and will be up later"
                echo "If you continue these will be deleted from your system"

                read -p "Remove files? [y/n] " cont_1
                    # If yes, remove broken PPA(s) from system
                           if [ "$cont_1" == "y" ]; then
                            clear
                            cat update_app.txt | grep Err | cut -c 12- | cut -d ' ' -f 1 > broken_app.txt 
                            IFS=$'\n' read -d '' -r -a lines < broken_app.txt
                            i="0"

                            while [[ "${lines[$i]}" != "" ]]; do
                                     grep -H "${lines[$i]}" /etc/apt/sources.list.d/*.list | cut -d ':' -f 1  > delete_app.txt
                                    IFS=$'\n' read -d '' -r -a linesd < delete_app.txt
                                    a="0"
                        
                                    while [[ "${linesd[$a]}" != "" ]]; do

                                        file="${linesd[$a]}"
                                        if [ -f "$file" ]; then
                                            clear
                                                echo "$file will be removed."
                                            read -p "Continue with removal? [y/n] " cont_2
                                            # If yes, remove $file
                                                if [ "$cont" == "y" ]; then
                                                        clear
                                                          echo "Removing $file";
                                                          sudo rm "$file"
                                                        sleep 5
                                                else
                                                          echo "Will keep $file";
                                                        sleep 5
                                                fi
                                        else
                                            clear
                                                echo "$file not found."
                                            sleep 5
                                        fi

                                        echo "${linesd[$a]}"
                                        a=$[$a+1]
                                    done
                   
                                i=$[$i+1]
                            done
                        elif [ "$cont_1" == "n" ]; then
                            clear
                            echo ""
                        fi
                clear
                rm update_app.txt
                rm broken_app.txt
                rm delete_app.txt
                
                # Unset old variables
                unset nls_maintain_ppa_check
                unset IFS
                unset cont_1
                unset cont_2
                unset file
                
            # Do not check, continue with maintenance    
            elif [ "$nls_maintain_ppa_check" == "n" ]; then
                unset nls_maintain_ppa_check
                echo "Not checking for broken PPAs"
            fi
        
        # System Maintenance
        clear
        sudo apt-get update
        sudo apt-get upgrade
        sudo apt-get install -f
        sudo apt-get upgrade
        sudo apt-get autoclean
        sudo apt-get autoremove
    
    }

    nls_init_menu

}
# Function for invalid menu selection error message
#
# This will happen if, for example, there are menu options 1, 2, 3, and 4, but the user
# selects option 5. There isn't an option 5, so the user will get this error message, and
# will be returned to the previous location to re-select a valid option
function nls_err_msg {

    clear
    echo "The option you chose is not valid. You must select a valid menu entry"
    sleep 2
    echo "You will be returned to your previous location, at which point, you will select"
    echo "a valid entry"
    echo ""
    read -p "Press any key to continue... "
    $nls_loc

}



# Developers:
#
#
# Tristan Williams    |    tristan401_2000@live.com
# Main script writer

# Changelog:
#
# 1 -    Initial script creation
# 2 -

As people contribute improvements to the script, I will have the updated script below:



Thank you all.

[TheFu]

Specify full paths to every command - always. This is rule #1 to scripting. It is a security thing AND a way to ensure someone with a strange PATH doesn't get screwed running the script.

Hardcoding filenames is poor for maintainability. Just like with program executables, it is best to set variables for those things in the beginning of the script. There are millions of examples of this. Here's a link to one of mine: http://blog.jdpfu.com/2013/02/23/cle...rnels-from-apt

Add a license so people know how you are willing for the code to be used. Nothing fancy is needed, but if you want the program to be used everywhere, some companies have specific policies which allow certain licenses and reject others.

Debian has recommended aptitude over apt-get for years.

There are a few traps I'd want to handle different signals at unexpected times of the script too.

I'll keep the programming style comments to myself.

[lykwydchykyn]

Just a suggestion, it might be easier for people to contribute to something like this if you put it on github or a similar code-collaboration site.

[ian-weisser]

I have real heartburn about encouraging beginners to use PPAs...the source of most package manager problems.

I'm not sure about the apt maintenance section. What kinds of issues it it supposed to solve?

apt-get update is already run by a daily cron job (/etc/cron.daily/apt)
apt-get upgrade is already run by the same cron job if the user's apt-conf settings are correct.
I find apt-get install -f to be only occasionally useful, and more frequently confusing to users than helpful.
I suspect that apt-get autoclean does something rather different than you expect. Again, it's already automatically done by the the daily cron job if the user hasn't been mucking with apt-conf settings.
apt-get autoremove can be done by the daily cron job. The settings have it turned off to prevent essential packages from getting autoremoved in error. It might be easier to simply enable it than to make it part of this script.

[papibe]

Hi Tristan_Williams.

I would remove all sudos from your script, and code it assuming it will be run as root:

sudo yourscrip.sh

You could check at the beginning of the script if it's being run as root:

#!/bin/bash

# Obtain current effective user ID.
CURRENT_EUID=$(id --user)

# Quit if the script is not being run by root.
if [ "$CURRENT_EUID" -ne 0 ]; then
    echo "$0 must be run as root. Example: sudo $0"
    exit 1
fi

Just my thoughts.
Regards.

[Erik1984]

- Move the code to github, launchpad or another code hosting service with syntax highlighting for bash.
- Split the functionality in multiple files or use more small functions. This reduces the visual complexity of function like nls_no_bkup_maintain with deep nesting.

[TheFu]

github would be preferred by more people outside Ubuntu than launchpad. For example, I do not have a launchpad account, but I do have github, which I don't use. I prefer to host my own git repo and share it as necessary, but for something like this - github would be preferred, probably. There are other options too - bitbucket.

Any function longer than 1 screen is TOO long. Simplify. That means 20 lines or less, including comments. Longer functions have been proven to hide bugs in other languages - don't know about bash.

[srikailash]

a new function whick checks wheather the available space in the drive entered by the user has enough available space to back up the files.This can be called after the drive name is entered.
function goes something like this::

function nls_checkspace {
wanted_space=$(df -h | awk '{if ($1 == "home folder path") print $4}')
avail_space=$(df -h | awk '{if ($1 == "backup drive path") print $4}')
#compare wanted space and available space
}