Browse code

introduce if/then & for/do rules

we mostly have a consistent style on if/then & for/do in devstack,
except when we don't. This attempts to build a set of rules to
enforce this.

Because there are times when lines are legitimately long, and there
is a continuation, this starts off ignoring if and for loops with
continuations. But for short versions, we should enforce this.

Changes to make devstack pass are included. The fact that the
cleanup patch was so small is pretty solid reason that this is
actually the style we've all agreed to.

Part of a git stash from hong kong that I finally cleaned up.

Change-Id: I6376d7afd59cc5ebba9ed69e5ee784a3d5934a10

Sean Dague authored on 2014/02/03 09:10:54
Showing 12 changed files
... ...
@@ -431,8 +431,7 @@ function upload_baremetal_image() {
431 431
 
432 432
 function clear_baremetal_of_all_nodes() {
433 433
     list=$(nova baremetal-node-list | awk -F '| ' 'NR>3 {print $2}' )
434
-    for node in $list
435
-    do
434
+    for node in $list; do
436 435
         nova baremetal-node-delete $node
437 436
     done
438 437
 }
... ...
@@ -186,8 +186,7 @@ function disk_image_create {
186 186
     local elements=$2
187 187
     local arch=$3
188 188
     local output=$TOP_DIR/files/$4
189
-    if [[ -f "$output.qcow2" ]];
190
-    then
189
+    if [[ -f "$output.qcow2" ]]; then
191 190
         echo "Image file already exists: $output_file"
192 191
     else
193 192
         ELEMENTS_PATH=$elements_path disk-image-create \
... ...
@@ -44,16 +44,14 @@ function neutron_plugin_configure_plugin_agent() {
44 44
 function neutron_plugin_configure_service() {
45 45
     iniset /$Q_PLUGIN_CONF_FILE restproxy servers $BS_FL_CONTROLLERS_PORT
46 46
     iniset /$Q_PLUGIN_CONF_FILE restproxy servertimeout $BS_FL_CONTROLLER_TIMEOUT
47
-    if [ "$BS_FL_VIF_DRIVER" = "ivs" ]
48
-    then
47
+    if [ "$BS_FL_VIF_DRIVER" = "ivs" ]; then
49 48
         iniset /$Q_PLUGIN_CONF_FILE nova vif_type ivs
50 49
     fi
51 50
 }
52 51
 
53 52
 function neutron_plugin_setup_interface_driver() {
54 53
     local conf_file=$1
55
-    if [ "$BS_FL_VIF_DRIVER" = "ivs" ]
56
-    then
54
+    if [ "$BS_FL_VIF_DRIVER" = "ivs" ]; then
57 55
         iniset $conf_file DEFAULT interface_driver neutron.agent.linux.interface.IVSInterfaceDriver
58 56
     else
59 57
         iniset $conf_file DEFAULT interface_driver neutron.agent.linux.interface.OVSInterfaceDriver
... ...
@@ -106,8 +106,7 @@ function _neutron_setup_ovs_tunnels() {
106 106
     local id=0
107 107
     GRE_LOCAL_IP=${GRE_LOCAL_IP:-$HOST_IP}
108 108
     if [ -n "$GRE_REMOTE_IPS" ]; then
109
-        for ip in ${GRE_REMOTE_IPS//:/ }
110
-        do
109
+        for ip in ${GRE_REMOTE_IPS//:/ }; do
111 110
             if [[ "$ip" == "$GRE_LOCAL_IP" ]]; then
112 111
                 continue
113 112
             fi
... ...
@@ -24,8 +24,7 @@ function init_bigswitch_floodlight() {
24 24
     sudo ovs-vsctl --no-wait br-set-external-id ${OVS_BRIDGE} bridge-id ${OVS_BRIDGE}
25 25
 
26 26
     ctrls=
27
-    for ctrl in `echo ${BS_FL_CONTROLLERS_PORT} | tr ',' ' '`
28
-    do
27
+    for ctrl in `echo ${BS_FL_CONTROLLERS_PORT} | tr ',' ' '`; do
29 28
         ctrl=${ctrl%:*}
30 29
         ctrls="${ctrls} tcp:${ctrl}:${BS_FL_OF_PORT}"
31 30
     done
... ...
@@ -1124,8 +1124,8 @@ fi
1124 1124
 # Create a randomized default value for the keymgr's fixed_key
1125 1125
 if is_service_enabled nova; then
1126 1126
     FIXED_KEY=""
1127
-    for i in $(seq 1 64);
1128
-        do FIXED_KEY+=$(echo "obase=16; $(($RANDOM % 16))" | bc);
1127
+    for i in $(seq 1 64); do
1128
+        FIXED_KEY+=$(echo "obase=16; $(($RANDOM % 16))" | bc);
1129 1129
     done;
1130 1130
     iniset $NOVA_CONF keymgr fixed_key "$FIXED_KEY"
1131 1131
 fi
... ...
@@ -49,8 +49,7 @@ function test_enable_service() {
49 49
 
50 50
     ENABLED_SERVICES="$start"
51 51
     enable_service $add
52
-    if [ "$ENABLED_SERVICES" = "$finish" ]
53
-    then
52
+    if [ "$ENABLED_SERVICES" = "$finish" ]; then
54 53
         echo "OK: $start + $add -> $ENABLED_SERVICES"
55 54
     else
56 55
         echo "changing $start to $finish with $add failed: $ENABLED_SERVICES"
... ...
@@ -76,8 +75,7 @@ function test_disable_service() {
76 76
 
77 77
     ENABLED_SERVICES="$start"
78 78
     disable_service "$del"
79
-    if [ "$ENABLED_SERVICES" = "$finish" ]
80
-    then
79
+    if [ "$ENABLED_SERVICES" = "$finish" ]; then
81 80
         echo "OK: $start - $del -> $ENABLED_SERVICES"
82 81
     else
83 82
         echo "changing $start to $finish with $del failed: $ENABLED_SERVICES"
... ...
@@ -102,8 +100,7 @@ echo "Testing disable_all_services()"
102 102
 ENABLED_SERVICES=a,b,c
103 103
 disable_all_services
104 104
 
105
-if [[ -z "$ENABLED_SERVICES" ]]
106
-then
105
+if [[ -z "$ENABLED_SERVICES" ]]; then
107 106
     echo "OK"
108 107
 else
109 108
     echo "disabling all services FAILED: $ENABLED_SERVICES"
... ...
@@ -118,8 +115,7 @@ function test_disable_negated_services() {
118 118
 
119 119
     ENABLED_SERVICES="$start"
120 120
     disable_negated_services
121
-    if [ "$ENABLED_SERVICES" = "$finish" ]
122
-    then
121
+    if [ "$ENABLED_SERVICES" = "$finish" ]; then
123 122
         echo "OK: $start + $add -> $ENABLED_SERVICES"
124 123
     else
125 124
         echo "changing $start to $finish failed: $ENABLED_SERVICES"
... ...
@@ -21,9 +21,19 @@
21 21
 # Currently Supported checks
22 22
 #
23 23
 # Errors
24
+# Basic white space errors, for consistent indenting
24 25
 # - E001: check that lines do not end with trailing whitespace
25 26
 # - E002: ensure that indents are only spaces, and not hard tabs
26 27
 # - E003: ensure all indents are a multiple of 4 spaces
28
+#
29
+# Structure errors
30
+#
31
+# A set of rules that help keep things consistent in control blocks.
32
+# These are ignored on long lines that have a continuation, because
33
+# unrolling that is kind of "interesting"
34
+#
35
+# - E010: *do* not on the same line as *for*
36
+# - E011: *then* not on the same line as *if*
27 37
 
28 38
 import argparse
29 39
 import fileinput
... ...
@@ -51,6 +61,23 @@ def print_error(error, line):
51 51
     print(" - %s: L%s" % (fileinput.filename(), fileinput.filelineno()))
52 52
 
53 53
 
54
+def not_continuation(line):
55
+    return not re.search('\\\\$', line)
56
+
57
+def check_for_do(line):
58
+    if not_continuation(line):
59
+        if re.search('^\s*for ', line):
60
+            if not re.search(';\s*do(\b|$)', line):
61
+                print_error('E010: Do not on same line as for', line)
62
+
63
+
64
+def check_if_then(line):
65
+    if not_continuation(line):
66
+        if re.search('^\s*if \[', line):
67
+            if not re.search(';\s*then(\b|$)', line):
68
+                print_error('E011: Then non on same line as if', line)
69
+
70
+
54 71
 def check_no_trailing_whitespace(line):
55 72
     if re.search('[ \t]+$', line):
56 73
         print_error('E001: Trailing Whitespace', line)
... ...
@@ -100,6 +127,8 @@ def check_files(files):
100 100
 
101 101
         check_no_trailing_whitespace(logical_line)
102 102
         check_indents(logical_line)
103
+        check_for_do(logical_line)
104
+        check_if_then(logical_line)
103 105
 
104 106
 
105 107
 def get_options():
... ...
@@ -194,8 +194,7 @@ function wait_for_VM_to_halt() {
194 194
     while true
195 195
     do
196 196
         state=$(xe_min vm-list name-label="$GUEST_NAME" power-state=halted)
197
-        if [ -n "$state" ]
198
-        then
197
+        if [ -n "$state" ]; then
199 198
             break
200 199
         else
201 200
             echo -n "."
... ...
@@ -63,8 +63,7 @@ get_params()
63 63
                 ;;
64 64
         esac
65 65
     done
66
-    if [[ -z $BRIDGE ]]
67
-    then
66
+    if [[ -z $BRIDGE ]]; then
68 67
         BRIDGE=xenbr0
69 68
     fi
70 69
 
... ...
@@ -91,8 +90,7 @@ xe_min()
91 91
 find_network()
92 92
 {
93 93
     result=$(xe_min network-list bridge="$1")
94
-    if [ "$result" = "" ]
95
-    then
94
+    if [ "$result" = "" ]; then
96 95
         result=$(xe_min network-list name-label="$1")
97 96
     fi
98 97
     echo "$result"
... ...
@@ -121,8 +119,7 @@ destroy_vifs()
121 121
 {
122 122
     local v="$1"
123 123
     IFS=,
124
-    for vif in $(xe_min vif-list vm-uuid="$v")
125
-    do
124
+    for vif in $(xe_min vif-list vm-uuid="$v"); do
126 125
         xe vif-destroy uuid="$vif"
127 126
     done
128 127
     unset IFS
... ...
@@ -7,8 +7,7 @@ declare -a on_exit_hooks
7 7
 
8 8
 on_exit()
9 9
 {
10
-    for i in $(seq $((${#on_exit_hooks[*]} - 1)) -1 0)
11
-    do
10
+    for i in $(seq $((${#on_exit_hooks[*]} - 1)) -1 0); do
12 11
         eval "${on_exit_hooks[$i]}"
13 12
     done
14 13
 }
... ...
@@ -17,8 +16,7 @@ add_on_exit()
17 17
 {
18 18
     local n=${#on_exit_hooks[*]}
19 19
     on_exit_hooks[$n]="$*"
20
-    if [[ $n -eq 0 ]]
21
-    then
20
+    if [[ $n -eq 0 ]]; then
22 21
         trap on_exit EXIT
23 22
     fi
24 23
 }
... ...
@@ -227,16 +227,14 @@ function test_get_local_sr_path {
227 227
 }
228 228
 
229 229
 [ "$1" = "run_tests" ] && {
230
-    for testname in $($0)
231
-    do
230
+    for testname in $($0); do
232 231
         echo "$testname"
233 232
         before_each_test
234 233
         (
235 234
             set -eux
236 235
             $testname
237 236
         )
238
-        if [ "$?" != "0" ]
239
-        then
237
+        if [ "$?" != "0" ]; then
240 238
             echo "FAIL"
241 239
             exit 1
242 240
         else