staging.inyokaproject.org

Für diese Funktion musst du eingeloggt sein.

Skript Optimierung, Review, Clean Code

Status: Ungelöst | Ubuntu-Version: Server 20.04 (Focal Fossa)
Antworten |

Herbststurm

Anmeldungsdatum:
27. Mai 2011

Beiträge: 244

Hallo

Ich habe mir ein bash Skript gebastelt (es funktioiert ☺), um auf einem neuen Ubuntu Server microk8s zu starten. Leider ist meine bash Erfahrung nicht so wie sie sein sollte... ☹ und aus diesem Grund möchte ich darum möchte ich euch bitten folgendes auf Code Qualität zu prüfen, und mir zu sagen was ich anders, oder besser machen soll (und warum).

Und ich habe eine Frage zu Zeile 131, wo die Firewall auf allow policy gestellt wird: Ich habe mehrere network interfaces in dem Server. Die Befehle die ich fand gaben mit immer alle interfaces zurück. Aber nur dieses eine, in meinem Fall venet0, konnte ich nicht bekommen. Wie mache ich das, um dies via Variable in das Skript einzubauen? Denn wenn ich auf einem anderen Server bin ist ja keineswegs sichergestellt, dass die Netzwerk Schnittstelle den selben Namen hat.

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
#/bin/bash
echo -e $"\n==================================================================================================================================="
echo -e $'\nTL;DR'
echo -e $'\nThis script is preparing a new blank linux server into a production ready kubernetes cluster for hosting containerized apps.'
echo -e $'\nControl bools has to be defined, because this script will be used ( or abused :) ) for more universal linux configurations too.'
echo -e $"\n==================================================================================================================================="

# Define the control bools
dummyVGA="True"  # if True a dummy X11 will be installed, to simulate a monitor which is useful in some docker applications
dummyXconf=/usr/share/X11/xorg.conf.d/xorg.conf
dockerCanonicalFork="True"  # if True the docker fork from Canonical, and if False the official docker will be installed.
dockerDeamon=/etc/docker/daemon.json
kubernetes="True" # if True Canonical microk8s will be installed. Use Canonical docker, because microk8s is from Canonical too. 

# root user creates a sudo user ubuntu if not already exists
if [ $USER = "root" ]; then
    if id -u "ubuntu" >/dev/null 2>&1; then
	echo -e $'\nUser ubuntu already has an user account'
    else
        echo -e $"\nInit new sudo user ubuntu"
        apt-get update
        apt-get install -y sudo
        adduser -m -s /bin/bash ubuntu
        adduser --gecos '' ubuntu
        usermod -aG sudo ubuntu
        echo "me  ALL=(ALL) NOPASSWD:ALL" | sudo tee /etc/sudoers.d/ubuntu
    fi #if ubuntu
    echo -e $'\nLogout and login as ubuntu or pi user, and re-execute this script'
fi #if root

# configure ubuntu user environment
if [ $USER = "ubuntu" ] || [ $USER = "pi" ]; then
    echo -e $"\nInstalling, or updating basic dependencies"
    sudo apt-get update
    sudo apt-get install -y ccache dos2unix bash-completion snapd fuse ufw nano nfs-common
    sudo systemctl start snapd.socket

    # Simulated monitor pluged in for dummy XServer 
    if test -f "$dummyXconf" && [[ $dummyVGA == "True" ]]; then
        echo -e $"\n==================================================="
	echo -e $"\nDummy XServer already installed"	
    else
        if [[ $dummyVGA == "True" ]]; then
            echo -e $"\n==================================================="
            echo -e $"\nInstall dummy vga monitor"
            sudo apt-get install -y xserver-xorg-video-dummy
            sudo touch $dummyXconf
            #ToDo sed mock X11 config into file
        fi
    fi

    # Docker section
    if [ -x "$(command -v docker)" ]; then
        echo -e $"\n==================================================="
	echo -e $"\ndocker already installed"	
        echo -e $"\n==================================================="
    else
        if [[ $dockerCanonicalFork == "False" ]]; then
            echo -e $"\nInstall official docker"
            echo -e $"\nRemove docker artefacts if they exist"
	    sudo apt-get remove docker docker-engine docker.io containerd runc
	    sudo apt-get purge docker-ce docker-ce-cli containerd.io
	    sudo rm -rf /var/lib/docker
	    sudo rm -rf /var/lib/containerd
            echo -e $"\nSetup docker repository"  
	    sudo apt-get install -y apt-transport-https ca-certificates curl gnupg lsb-release
	    curl -fsSL https://download.docker.com/linux/ubuntu/gpg | sudo gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg
		    echo \
	  		   "deb [arch=amd64 signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/ubuntu \
  			   $(lsb_release -cs) stable" | sudo tee /etc/apt/sources.list.d/docker.list > /dev/null
  	    sudo apt-get update

	    echo -e $"\nInstall official docker"
	    sudo apt-get install -y docker-ce docker-ce-cli containerd.io

	    echo -e $"\nManage docker as non-root user"
	    sudo groupadd docker
	    sudo usermod -aG docker $USER
	    sudo chown "$USER":"$USER" /home/"$USER"/.docker -R
	    sudo chmod g+rwx "$HOME/.docker" -R

	    echo -e $"\nConfigure docker to start on boot"
	    sudo systemctl enable docker.service
	    sudo systemctl enable containerd.service

	    echo -e "\nInstall docker-compose"
	    sudo curl -L "https://github.com/docker/compose/releases/download/1.29.2/docker-compose-$(uname -s)-$(uname -m)" -o /usr/local/bin/docker-compose
	    sudo chmod +x /usr/local/bin/docker-compose
	    sudo ln -s /usr/local/bin/docker-compose /usr/bin/docker-compose

	    echo -e "\nNext you have to setup systemd for docker!"
	    echo "See: Configuring remote access with systemd unit file in https://docs.docker.com/engine/install/linux-postinstall/"
	    echo -e "\ndocker successfully installed. please logout and login again"
            echo -e $"\n==================================================="
	else
            echo -e $"\n==================================================="
	    echo -e $"\nInstall Canonical docker"
	    sudo apt-get install -y docker.io docker-compose docker-doc docker-registry
	    echo -e $"\nManage docker as non-root user"
	    sudo groupadd docker
	    sudo usermod -aG docker $USER
            sudo chown ubuntu:docker /var/run/docker.sock
            sudo chown -R ubuntu:docker /var/run/docker
            sudo systemctl enable  docker
            echo -e $'\nLogin as ubuntu or pi user, and re-execute this script'
            echo -e $'\nSystem will be rebootet now'
            sudo reboot
        fi
    fi #if docker

    # Kubernetes section
    if [[ $kubernetes == "True" ]]; then
        if [ -x "$(command -v microk8s)" ]; 
        then
            echo -e $"\n==================================================="
            echo -e $"\nmicrok8s already installed"  
            echo -e $"\n==================================================="
        else
            if [ -f "$dockerDeamon" ]; then
                echo -e $"\nDocker daemon already configured"	
                echo "$dockerDeamon exists."
            else 
                echo "$dockerDeamon does not exist."
                # because of microk8s inspect 'WARNING:  Docker is installed.'
                sudo touch /etc/docker/daemon.json
                sudo tee /etc/docker/daemon.json <<<'{"insecure-registries":["localhost:32000"]}'
                sudo systemctl restart docker
            fi

            echo -e $"\nConfigure firewall for Kubernetes"
            sudo ufw allow in on venet0 && sudo ufw allow out on venet0
            sudo ufw default allow routed

            echo -e $"\n==================================================="
            echo -e $"\nDisabeling swap for Kubernetes"
            sudo swapoff -a

            echo -e $"\n==================================================="
            echo -e $"\nSee what versions of microk8s are currently published"       
            snap info microk8s

            echo -e $"\n==================================================="
            echo -e $"\nInstall microk8s - Canocical Kubernetes"  
            sudo snap install microk8s --classic --channel=1.18/stable
            sudo usermod -a -G microk8s $USER      

            echo -e $"\nGenerate and show microk8s kube config"
            mkdir -p ~/.kube
            microk8s config > ~/.kube/config
            microk8s config
            sudo chown -R $USER. ~/.kube

            echo -e $"\n==================================================="
            echo -e $"\nShow kubelet service status"
            systemctl status snap.microk8s.daemon-kubelet

            echo -e $"\n==================================================="
            echo -e $"\nEnable addons and basic services"  
            microk8s.enable dns dashboard storage registry        

            echo -e $"\nmicrok8s waiting for status"  
            microk8s status --wait-ready

            echo -e $"\n==================================================="
            echo -e $"\nGet microk8s namespaces"  
            microk8s.kubectl get all --namespace kube-system

            echo -e $"\n==================================================="
            echo -e $"\nmicrok8s inspect"
            microk8s inspect

            echo -e $"\n==================================================="
            echo -e $"\nCheck kubernetes ports"
            sudo netstat -lnpt|grep kube           

            echo -e $"\n==================================================="
            echo -e $"\nCreate microk8s access token"      
            token=$(microk8s kubectl -n kube-system get secret | grep default-token | cut -d " " -f1)
            microk8s kubectl -n kube-system describe secret $token
            
            echo -e $"\nmicrok8s dashboard IP"                  
            microk8s kubectl -n kube-system get service/kubernetes-dashboard -o go-template={{.spec.clusterIP}}

            echo -e $"\n==================================================="
        fi
    fi #if kubernetes
fi #if ubuntu

Vielen Dank und viele Grüße

user_unknown

Avatar von user_unknown

Anmeldungsdatum:
10. August 2005

Beiträge: 17630

Ich kann zum eigentlichen Job wenig sagen, da ich mit Docker/Kubernetes noch nichts gemacht habe.

Aber zu manchen Kleinigkeiten kann ich mich äußern.

Wieso setzt Du das Wort True (und viele andere) in Anführungsstriche?

1
dummyVGA="True" 

Was sollen die Anführungsstriche bewirken? Was bewirken sie wirklich? Wann braucht man sie?

Hier frage ich mich zweierlei:

1
echo -e $'\nUser ubuntu already has an user account'

Wieso der Wechsel zu Apostrophen? Und was soll das führende Dollarzeichen bewirken?

1
2
3
echo -e $"\n==================================================="
echo -e $"\ndocker already installed"	
echo -e $"\n==================================================="

Umrandung als Hervorhebung, und dennoch eine Extrazeile Abstand? Wenn man jede Zeile hervorhebt ist es keine Hervorhebung mehr.

Dem Shebang fehlt ein Ausrufezeichen.

Kommentierte fis sind ein Codesmell, und zwar riecht es nach zu viel Code in einer Verzweigung, wenn man am Ende den Anfang nicht mehr sieht.

Eine mechanische Lösung wäre es, alle Blöcke in eigene Funktionen auszulagern, und die Namen dafür finden sich eigentlich schon in den sie begleitenden Blöcken:

Z. 176-179:

1
2
3
4
5

            echo -e $"\n==================================================="
            echo -e $"\nCreate microk8s access token"      
            token=$(microk8s kubectl -n kube-system get secret | grep default-token | cut -d " " -f1)
            microk8s kubectl -n kube-system describe secret $token
 1
 2
 3
 4
 5
 6
 7
 8
 9
10
createMicrok8sAccessToken () {
            echo "==================================================="
            echo "Create microk8s access token"      
            token=$(microk8s kubectl -n kube-system get secret | grep default-token | cut -d " " -f1)
            microk8s kubectl -n kube-system describe secret $token
}
#
# ... viele Zeilen später:
#
            createMicrok8sAccessToken ()

kB Team-Icon

Supporter, Wikiteam
Avatar von kB

Anmeldungsdatum:
4. Oktober 2007

Beiträge: 9837

Herbststurm schrieb:

[…] auf Code Qualität zu prüfen […]

[ $USER = "root" ]

Obwohl es in Deinem Skript problemlos funktioniert, ist dies grundsätzlich ein schwerer programmiertechnischer Kunstfehler!

Ein Problem entsteht, wenn die Variable nicht gesetzt oder leer ist. (Das kann zwar explizit bei der bash mit der Variable USER nicht geschehen, aber in anderen Situationen schon.) Dann wird dieser Test einen syntaktischen Fehler erzeugen und Dein Programm auf jeden Fall mit dem folgenden Befehl weitermachen. Damit wird Code auch in Situationen ausgeführt, für die er nicht vorgesehen ist.

Das richtige Idiom für Tests in der Shell (bash und andere) sieht so aus:

 [ "$VARIABLE" = Suchstring ] 

bzw. in Deinem Fall

 [ "$USER" = root ] 

Wenn jetzt die Variable keinen Wert hat, lautet das explizit

 [ "" = root ] 

und das ergibt falsch. Damit wird der von diesem Test abhängige folgende Code sicher nicht ausgeführt.

Quotierung des Suchstrings ist nur erforderlich, wenn er Leerzeichen oder andere problematische Sonderzeichen enthält. Quotierung der Variable ist dagegen immer erforderlich. Nun ist zwar USER ein Sonderfall, weil immer ein Wert vorliegt, aber Du hast ja nach Clean Code gefragt.

rklm Team-Icon

Projektleitung

Anmeldungsdatum:
16. Oktober 2011

Beiträge: 13242

Das erste was mir auffällt sind die Massen von sudo. Es erscheint, man führt das Skript besser als "root" aus. Das kann man z.B. so automatisieren:

1
2
3
4
5
6
7
8
9
#!/bin/sh

[ "$(id -u)" -eq 0 ] || exec sudo "$0" "$@"

# Als root
id

# Jetzt als der aufrufende User:
sudo -u "$SUDO_USER" id

Zum Debuggen ist set -x sehr nützlich. Ich schalte das gerne durch eine Umgebungsvariable ein:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
#!/bin/sh

[ -z "$DEBUG" ] || set -x
[ $(id -u) -eq 0 ] || exec sudo "DEBUG=$DEBUG" "$0" "$@"

# Kommando als root
id

# Jetzt als der ursprünglich aufrufende User:
sudo -u "$SUDO_USER" id

id dient nur der Illustration, welcher User gerade aktiv ist. Zeile 32 muss dann natürlich SUDO_USER prüfen und nicht USER.

user_unknown schrieb:

Hier frage ich mich zweierlei:

1
echo -e $'\nUser ubuntu already has an user account'

Wieso der Wechsel zu Apostrophen? Und was soll das führende Dollarzeichen bewirken?

Das ist eine spezielle Syntax der bash, die dazu führt, dass Sequenzen wie "\n" übersetzt werden (hier zu einem Newline):

1
2
3
4
5
6
7
8
$ echo 'foo\nbar' | od -t x1c
0000000  66  6f  6f  5c  6e  62  61  72  0a
          f   o   o   \   n   b   a   r  \n
0000011
$ echo $'foo\nbar' | od -t x1c
0000000  66  6f  6f  0a  62  61  72  0a
          f   o   o  \n   b   a   r  \n
0000010
1
2
3
echo -e $"\n==================================================="
echo -e $"\ndocker already installed"	
echo -e $"\n==================================================="

Bei so etwas würde sich auch eher ein here document anbieten. Man könnte solche Ausgaben auch in eine Funktion auslagern. Das vermindert Redundanz und macht eine spätere Änderung leichter (z.B. wenn einem "/////" besser gefällt als "====="). Insgesamt ist da sehr viel Wiederholung in dem Skript. Die würde ich auf jeden Fall loswerden.

Dein Skript macht sehr verschiedene Dinge abhängig vom Nutzer, der das ausführt ("root" vs. "ubuntu" oder "pi"). Das halte ich für schlecht, weil es die Regel verletzt, dass ein Artefakt möglichst eine Sache machen soll. Ich würde das auf jeden Fall auf zwei Skripte verteilen.

Und in dem Skript, das die Nicht-root-Arbeit macht, würde ich bei "falschem" Nutzer schnell aussteigen. Das macht den Riesen-if überflüssig:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
#!/bin/sh

abort() { echo "$*" >&2; exit 1; }

# Wenn man ein bisschen paranoid ist:
export USER=$(id -un)

if [ "$USER" != ubuntu -a "$USER" != pi ]; then
  abort "ERROR: user needs to be 'ubuntu' or 'pi' but was '$USER'"
fi

...

user_unknown

Avatar von user_unknown

Anmeldungsdatum:
10. August 2005

Beiträge: 17630

rklm schrieb:

user_unknown schrieb:

Hier frage ich mich zweierlei:

1
echo -e $'\nUser ubuntu already has an user account'

Wieso der Wechsel zu Apostrophen? Und was soll das führende Dollarzeichen bewirken?

Das ist eine spezielle Syntax der bash, die dazu führt, dass Sequenzen wie "\n" übersetzt werden (hier zu einem Newline):

1
2
3
4
5
6
7
8
$ echo 'foo\nbar' | od -t x1c
0000000  66  6f  6f  5c  6e  62  61  72  0a
          f   o   o   \   n   b   a   r  \n
0000011
$ echo $'foo\nbar' | od -t x1c
0000000  66  6f  6f  0a  62  61  72  0a
          f   o   o  \n   b   a   r  \n
0000010

Da ist aber schon der -e-Schalter gesetzt, der ein einen Zeilenumbruch erzeugt:

1
2
3
4
5
6
7
8
t530:~/job/fbb/mysql 🐧> echo -e 'foo\nbar' | od -t x1c
0000000  66  6f  6f  0a  62  61  72  0a
          f   o   o  \n   b   a   r  \n
0000010
t530:~/job/fbb/mysql 🐧> echo -e $'foo\nbar' | od -t x1c
0000000  66  6f  6f  0a  62  61  72  0a
          f   o   o  \n   b   a   r  \n
0000010

user_unknown

Avatar von user_unknown

Anmeldungsdatum:
10. August 2005

Beiträge: 17630

kB schrieb:

Herbststurm schrieb:

[…] auf Code Qualität zu prüfen […]

[ $USER = "root" ]

Obwohl es in Deinem Skript problemlos funktioniert, ist dies grundsätzlich ein schwerer programmiertechnischer Kunstfehler!

Das sehe ich aber anders.

Eine Regel stur anzuwenden, auch wo der Grund hinfällig ist, ist Dogmatismus und das Gegenteil von Kunst.

user_unknown

Avatar von user_unknown

Anmeldungsdatum:
10. August 2005

Beiträge: 17630

Wo wir bei Kunst sind -

In The art of unix programming hat Eric S. Raymond festgehalten:

Rule of Silence: When a program has nothing surprising to say, it should say nothing.

One of Unix's oldest and most persistent design rules is that when a program has nothing interesting or surprising to say, it should shut up. Well-behaved Unix programs do their jobs unobtrusively, with a minimum of fuss and bother. Silence is golden.

This “silence is golden” rule evolved originally because Unix predates video displays. On the slow printing terminals of 1969, each line of unnecessary output was a serious drain on the user's time. That constraint is gone, but excellent reasons for terseness remain.

I think that the terseness of Unix programs is a central feature of the style. When your program's output becomes another's input, it should be easy to pick out the needed bits. And for people it is a human-factors necessity — important information should not be mixed in with verbosity about internal program behavior. If all displayed information is important, important information is easy to find.

– Ken Arnold

Well-designed programs treat the user's attention and concentration as a precious and limited resource, only to be claimed when necessary.

(We'll discuss the Rule of Silence and the reasons for it in more detail at the end of Chapter 11.)

Quelle: http://www.catb.org/~esr/writings/taoup/html/ch01s06.html#id2878450

Das Buch ist online frei erhältlich und sollte gelesen werden, wenn man sich für Codequalität interessiert.

Bearbeitet von noisefloor:

Syntax korrigiert, jetzt funktioniert der Link auch ☺ Externe Links werden nur in eckige Klammern eingeschlossen.[/mod]

rklm Team-Icon

Projektleitung

Anmeldungsdatum:
16. Oktober 2011

Beiträge: 13242

user_unknown schrieb:

Da ist aber schon der -e-Schalter gesetzt, der ein einen Zeilenumbruch erzeugt:

Ich habe auch nicht gesagt, dass das sinnvoll ist. ☺

Und auch, wenn man dieselbe Ausgabe erzielen kann, ist es trotzdem nicht das gleiche, weil $'..\n..' zu einer anderen Zeichenkette in der Variablen führt als '..\n..'. Das macht einen Unterschied, wenn es z.B. um einen Dateinamen geht, der tatsächlich einen Newline oder aber die Sequenz "\n" (zwei Zeichen / Bytes) enthält.

user_unknown schrieb:

Das Buch ist online frei erhältlich und sollte gelesen werden, wenn man sich für Codequalität interessiert.

Aber nur, wenn es etwas Überraschendes zu sagen hat. 😉

kB Team-Icon

Supporter, Wikiteam
Avatar von kB

Anmeldungsdatum:
4. Oktober 2007

Beiträge: 9837

user_unknown

Avatar von user_unknown

Anmeldungsdatum:
10. August 2005

Beiträge: 17630

kB schrieb:

user_unknown schrieb:

[…] Quelle: //www.catb.org/~esr/writings/taoup/html/ch01s06.html#id2878450:

Bitte den Link korrigieren!

Oh, selbstverständlich.

(Rule of silence) http://www.catb.org/~esr/writings/taoup/html/ch01s06.html#id2878450

rklm schrieb:

user_unknown schrieb:

Da ist aber schon der -e-Schalter gesetzt, der ein einen Zeilenumbruch erzeugt:

Ich habe auch nicht gesagt, dass das sinnvoll ist. ☺

Richtig, Du hast gesagt, besser geschrieben, es würde hier zu einem Newline, wobei das hier doppeldeutig ist, bezieht es sich auf hier den Code unter Review, oder hier, dass Beispiel hier, das Du dann bringst, und das eben mit meiner Frage nichts mehr zu tun hat, weil das -e unterschlagen wird, um einen Unterschied zu machen, der gar nicht zur Debatte stand.

Und auch, wenn man dieselbe Ausgabe erzielen kann, ist es trotzdem nicht das gleiche, weil $'..\n..' zu einer anderen Zeichenkette in der Variablen führt

Es gibt nur keine Variable.

als '..\n..'. Das macht einen Unterschied, wenn es z.B. um einen Dateinamen geht, der tatsächlich einen Newline oder aber die Sequenz "\n" (zwei Zeichen / Bytes) enthält.

Es gibt da auch keinen Dateinamen.

Also so recht weiß ich nicht, was Du mit Deinem Posting sagen wolltest - wozu das Dollarzeichen gut wäre, wenn der Code ein ganz anderer wäre, offenbar.

Antworten |