Merge lp:~jseutter/landscape-client/better-config-handling into lp:~landscape/landscape-client/trunk

[Jerry Seutter]

This branch modifies two things:
- client.conf is no longer backed up before modification. This is back to the old behavior after it was recently changed.
- client.conf is only written to by landscape-config. landscape-client.postinst no longer modifies client.conf. This prevents a situation where client.conf would only be partially rewritten.

[Chad Smith (chad.smith)]

+1 This branch behaves as you mentioned in the merge description.

tried feeding bogus values into ping_interval to validate that dpkg-reconfigure crashes and doesn't write the config file if some bogus option values live in /etc/landscape/client.conf and get passed to landscape-config cmdline.

Couple questions:
[1] Since we just now got rid of the static landscape-config header values for log_level & data_path, should these default values be added into debian/landscape-client.templates and pull a db_get $PACKAGE/data_path * log_level to pass on the cmdline to landscape-config?

68 -log_level = info
69 -data_path = /var/lib/landscape/client

[2] Why not validate if REGISTRATION_PASSWORD is non-null too before attempting a registration? I know we've got he --ok-no-register flag set but do we already allow empty registration passwords for clients?
118 + if [ -n "$ACCOUNT_NAME" -a -n "$COMPUTER_TITLE" ]; then

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Nice work, +1!

Regarding Chad's [1], I don't think we need to expose log_level and data_path in debconf, as changing the former is mainly meant for debugging and the latter for development. So I don't really see use cases for preseeding them, if one wants to change those values it's sufficient to edit them manually in client.conf and they will be picked up and preserved.

And regarding [2], the registration password is optional on both hosted and LDS, so the empty string is a legal value.

As for the crashing bogus values maybe we can address it in a separate branch, it doesn't seem critical as we don't prompt the user for those values, neither with debconf or the interactive landscape-config tool. Actually I'm wondering if we should include them in debconf at all.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Oh Jerry, one thing I forgot to ask. Does landscape-config re-write the file all the times or does it intelligently understand when there's no change to write?

[Free Ekanayaka (free.ekanayaka)]

Never mind, just noticed that you mention in the bug that it's going to be fixed in a separate branch.

[lp:~jseutter/landscape-client/better-config-handling]

458. By Jerry Seutter on 2012-02-24

Merge from trunk.

=== modified file 'debian/landscape-client.postinst'
--- debian/landscape-client.postinst	2012-02-13 21:07:42 +0000
+++ debian/landscape-client.postinst	2012-02-23 18:35:24 +0000
@@ -27,68 +27,57 @@
     configure)
 
         CONFIG_FILE=/etc/landscape/client.conf
-        if [ -f $CONFIG_FILE ]; then
-            mv --backup=numbered $CONFIG_FILE $CONFIG_FILE.bak
-        fi
+        # Get configuration values from debconf
+        db_get $PACKAGE/computer_title
+        COMPUTER_TITLE="${RET}"
+        db_get $PACKAGE/account_name
+        ACCOUNT_NAME="${RET}"
+        db_get $PACKAGE/registration_password
+        REGISTRATION_PASSWORD="${RET}"
+        db_get $PACKAGE/url
+        URL="${RET}"
+        db_get $PACKAGE/exchange_interval
+        EXCHANGE_INTERVAL="${RET}"
+        db_get $PACKAGE/urgent_exchange_interval
+        URGENT_EXCHANGE_INTERVAL="${RET}"
+        db_get $PACKAGE/ping_url
+        PING_URL="${RET}"
+        db_get $PACKAGE/ping_interval
+        PING_INTERVAL="${RET}"
+        db_get $PACKAGE/http_proxy
+        HTTP_PROXY="${RET}"
+        db_get $PACKAGE/https_proxy
+        HTTPS_PROXY="${RET}"
+        db_get $PACKAGE/otp
+        OTP="${RET}"
+        db_get $PACKAGE/tags
+        TAGS="${RET}"
+
         if [ ! -f $CONFIG_FILE ]; then
-            # Get configuration values from debconf
-            db_get $PACKAGE/computer_title
-            COMPUTER_TITLE="${RET}"
-            db_get $PACKAGE/account_name
-            ACCOUNT_NAME="${RET}"
-            db_get $PACKAGE/registration_password
-            REGISTRATION_PASSWORD="${RET}"
-            db_get $PACKAGE/url
-            URL="${RET}"
-            db_get $PACKAGE/exchange_interval
-            EXCHANGE_INTERVAL="${RET}"
-            db_get $PACKAGE/urgent_exchange_interval
-            URGENT_EXCHANGE_INTERVAL="${RET}"
-            db_get $PACKAGE/ping_url
-            PING_URL="${RET}"
-            db_get $PACKAGE/ping_interval
-            PING_INTERVAL="${RET}"
-            db_get $PACKAGE/http_proxy
-            HTTP_PROXY="${RET}"
-            db_get $PACKAGE/https_proxy
-            HTTPS_PROXY="${RET}"
-            db_get $PACKAGE/otp
-            OTP="${RET}"
-            db_get $PACKAGE/tags
-            TAGS="${RET}"
-
-            # Create new configuration, with private mode
-            TEMPFILE=$(mktemp -p /etc/landscape)
-            cat > $TEMPFILE <<END
-[client]
-log_level = info
-data_path = /var/lib/landscape/client
-END
-            chown landscape $TEMPFILE
-            mv $TEMPFILE $CONFIG_FILE
-
-            # Configure landscape
-            landscape-config --silent --no-start \
-                --computer-title "$COMPUTER_TITLE" \
-                --account-name "$ACCOUNT_NAME" \
-                --registration-password "$REGISTRATION_PASSWORD" \
-                --url "$URL" \
-                --exchange-interval "$EXCHANGE_INTERVAL" \
-                --urgent-exchange-interval "$URGENT_EXCHANGE_INTERVAL" \
-                --ping-url "$PING_URL" \
-                --ping-interval "$PING_INTERVAL" \
-                --http-proxy "$HTTP_PROXY" \
-                --https-proxy "$HTTPS_PROXY" \
-                --otp "$OTP" \
-                --tags "$TAGS"
-
-            # If we got the needed information, actually do the registration.
-            if [ -n "$ACCOUNT_NAME" -a -n "$COMPUTER_TITLE" ]; then
-                landscape-config --silent --ok-no-register
-            fi
-        else
-            # Fix non-private permissions
-            chmod 0600 $CONFIG_FILE
+            touch $CONFIG_FILE
+        fi
+        chown landscape $CONFIG_FILE
+        # Fix non-private permissions
+        chmod 0600 $CONFIG_FILE
+            
+        # Configure landscape
+        landscape-config --silent --no-start \
+            --computer-title "$COMPUTER_TITLE" \
+            --account-name "$ACCOUNT_NAME" \
+            --registration-password "$REGISTRATION_PASSWORD" \
+            --url "$URL" \
+            --exchange-interval "$EXCHANGE_INTERVAL" \
+            --urgent-exchange-interval "$URGENT_EXCHANGE_INTERVAL" \
+            --ping-url "$PING_URL" \
+            --ping-interval "$PING_INTERVAL" \
+            --http-proxy "$HTTP_PROXY" \
+            --https-proxy "$HTTPS_PROXY" \
+            --otp "$OTP" \
+            --tags "$TAGS"
+
+        # If we got the needed information, actually do the registration.
+        if [ -n "$ACCOUNT_NAME" -a -n "$COMPUTER_TITLE" ]; then
+            landscape-config --silent --ok-no-register
         fi
 
         # old monolithic clients have a single persist data file and a single