Merge lp:~frankban/juju-gui/bug-1229939-service-set into lp:juju-gui/experimental

[Francesco Banconi]

Fix settings update in juju-core.

When sending ServiceSet requests, the GUI included
a Config parameter where juju-core expects an
Option one.
While I was looking at that code, I decided instead to
switch to the newer ServiceUpdate API call, which
handles both map[string]string and YAML settings.

QA:
- deploy and expose the GUI in a juju-core env;
- switch to this branch:
    juju set juju-gui juju-gui-source=lp:~frankban/juju-gui/bug-1229939-service-set
- try to change settings (e.g. juju-gui-debug);
- refresh.

The settings viewlet should reflect the changes and,
after a while (after config-changed runs),
the new settings should be applied.

[Francesco Banconi (frankban)]

Reviewers: mp+187544_code.launchpad.net,

Message:
Please take a look.

Description:
Fix settings update in juju-core.

When sending ServiceSet requests, the GUI included
a Config parameter where juju-core expects an
Option one.
While I was looking at that code, I decided instead to
switch to the newer ServiceUpdate API call, which
handles both map[string]string and YAML settings.

QA:
- deploy and expose the GUI in a juju-core env;
- switch to this branch:
     juju set juju-gui
juju-gui-source=lp:~frankban/juju-gui/bug-1229939-service-set
- try to change settings (e.g. juju-gui-debug);
- refresh.

The settings viewlet should reflect the changes and,
after a while (after config-changed runs),
the new settings should be applied.

https://code.launchpad.net/~frankban/juju-gui/bug-1229939-service-set/+merge/187544

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13900044/

Affected files (+52, -61 lines):
   A [revision details]
   M app/store/env/go.js
   M app/store/env/sandbox.js
   M test/test_env_go.js
   M test/test_inspector_settings.js
   M test/test_sandbox_go.js
   M test/test_service_config_view.js

[Francesco Banconi (frankban)]

This bug has been fixed here: https://codereview.appspot.com/13917043/ .

https://codereview.appspot.com/13900044/

Unmerged revisions

1087. By Francesco Banconi on 2013-09-25

Checkpoint.

=== modified file 'app/store/env/go.js'
--- app/store/env/go.js	2013-09-13 21:49:15 +0000
+++ app/store/env/go.js	2013-09-25 15:43:51 +0000
@@ -1008,25 +1008,25 @@
           (!Y.Lang.isValue(config) && !Y.Lang.isValue(data))) {
         throw 'Exactly one of config and data must be provided';
       }
-      var intermediateCallback, sendData;
+      var intermediateCallback;
       if (callback) {
         // Capture the callback and serviceName.  No context is passed.
         intermediateCallback = Y.bind(this.handleServiceCalls, null,
             callback, serviceName);
       }
-      sendData = {
-        Type: 'Client',
-        Params: {ServiceName: serviceName}
-      };
+      var params = {ServiceName: serviceName};
       if (data) {
-        sendData.Request = 'ServiceSetYAML';
-        sendData.Params.ConfigYAML = data;
+        params.SettingsYAML = data;
       } else {
         config = utils.removeUnchangedConfigOptions(config, serviceConfig);
-        sendData.Request = 'ServiceSet';
-        sendData.Params.Config = stringifyObjectValues(config);
+        params.SettingsStrings = stringifyObjectValues(config);
       }
-      this._send_rpc(sendData, intermediateCallback);
+      var request = {
+        Type: 'Client',
+        Request: 'ServiceUpdate',
+        Params: params
+      };
+      this._send_rpc(request, intermediateCallback);
     },
 
     /**

=== modified file 'app/store/env/sandbox.js'
--- app/store/env/sandbox.js	2013-09-18 18:15:19 +0000
+++ app/store/env/sandbox.js	2013-09-25 15:43:51 +0000
@@ -1126,42 +1126,33 @@
     },
 
     /**
-    Handle ServiceSet messages
-
-    @method handleClientServiceSet
-    @param {Object} data The contents of the API arguments.
-    @param {Object} client The active ClientConnection.
-    @param {Object} state An instance of FakeBackend.
-    @return {undefined} Side effects only.
-    */
-    handleClientServiceSet: function(data, client, state) {
-      var result = state.setConfig(data.Params.ServiceName, data.Params.Config);
-      this._basicReceive(data, client, result);
-    },
-
-    /**
-    Handle ServiceSetYAML messages
-
-    @method handleClientServiceSetYAML
-    @param {Object} data The contents of the API arguments.
-    @param {Object} client The active ClientConnection.
-    @param {Object} state An instance of FakeBackend.
-    @return {undefined} Side effects only.
-    */
-    handleClientServiceSetYAML: function(data, client, state) {
-      var config = {};
-      try {
-        config = jsyaml.safeLoad(data.Params.ConfigYAML);
-      } catch (e) {
-        if (e instanceof jsyaml.YAMLException) {
-          this._basicReceive(data, client,
-              {error: 'Error parsing YAML.\n' + e});
-          return;
-        }
-        throw e;
-      }
+    Handle ServiceUpdate messages.
+
+    @method handleClientServiceUpdate
+    @param {Object} data The contents of the API arguments.
+    @param {Object} client The active ClientConnection.
+    @param {Object} state An instance of FakeBackend.
+    @return {undefined} Side effects only.
+    */
+    handleClientServiceUpdate: function(data, client, state) {
       var serviceName = data.Params.ServiceName;
-      var result = state.setConfig(serviceName, config[serviceName]);
+      var settings;
+      if (data.Params.SettingsYAML) {
+        try {
+          settings = jsyaml.safeLoad(data.Params.SettingsYAML);
+        } catch (e) {
+          if (e instanceof jsyaml.YAMLException) {
+            this._basicReceive(data, client,
+                {error: 'Error parsing YAML.\n' + e});
+            return;
+          }
+          throw e;
+        }
+        settings = settings[serviceName];
+      } else {
+        settings = data.Params.SettingsStrings;
+      }
+      var result = state.setConfig(serviceName, settings);
       this._basicReceive(data, client, result);
     },
 

=== modified file 'test/test_env_go.js'
--- test/test_env_go.js	2013-09-16 15:57:12 +0000
+++ test/test_env_go.js	2013-09-25 15:43:51 +0000
@@ -843,15 +843,13 @@
       });
       msg = conn.last_message();
       var expected = {
+        RequestId: msg.RequestId,
         Type: 'Client',
+        Request: 'ServiceUpdate',
         Params: {
           ServiceName: 'mysql',
-          Config: {
-            'cfg-key': 'cfg-val'
-          }
-        },
-        Request: 'ServiceSet',
-        RequestId: msg.RequestId
+          SettingsStrings: {'cfg-key': 'cfg-val'}
+        }
       };
       assert.deepEqual(expected, msg);
     });
@@ -865,10 +863,10 @@
       var expected = {
         RequestId: msg.RequestId,
         Type: 'Client',
-        Request: 'ServiceSetYAML',
+        Request: 'ServiceUpdate',
         Params: {
           ServiceName: 'mysql',
-          ConfigYAML: data
+          SettingsYAML: data
         }
       };
       assert.deepEqual(expected, msg);

=== modified file 'test/test_inspector_settings.js'
--- test/test_inspector_settings.js	2013-09-20 17:29:21 +0000
+++ test/test_inspector_settings.js	2013-09-25 15:43:51 +0000
@@ -326,7 +326,7 @@
         input, inspector.viewletManager.viewlets.config);
     button.simulate('click');
     var message = env.ws.last_message();
-    assert.equal('foo', message.Params.Config.admins);
+    assert.equal('foo', message.Params.SettingsStrings.admins);
     // Send back a success message.
     env.ws.msg({RequestId: message.RequestId});
     assert.equal(button.getHTML(), 'Save changes');

=== modified file 'test/test_sandbox_go.js'
--- test/test_sandbox_go.js	2013-09-18 18:15:19 +0000
+++ test/test_sandbox_go.js	2013-09-25 15:43:51 +0000
@@ -516,10 +516,10 @@
       state.deploy('cs:precise/wordpress-15', function() {
         var data = {
           Type: 'Client',
-          Request: 'ServiceSet',
+          Request: 'ServiceUpdate',
           Params: {
             ServiceName: 'wordpress',
-            Config: { engine: 'apache' }
+            SettingsStrings: { engine: 'apache' }
           },
           RequestId: 42
         };
@@ -564,10 +564,10 @@
       state.deploy('cs:precise/wordpress-15', function() {
         var data = {
           Type: 'Client',
-          Request: 'ServiceSetYAML',
+          Request: 'ServiceUpdate',
           Params: {
             ServiceName: 'wordpress',
-            ConfigYAML: 'wordpress:\n  engine: apache'
+            SettingsYAML: 'wordpress:\n  engine: apache'
           },
           RequestId: 42
         };

=== modified file 'test/test_service_config_view.js'
--- test/test_service_config_view.js	2013-09-17 13:27:43 +0000
+++ test/test_service_config_view.js	2013-09-25 15:43:51 +0000
@@ -161,11 +161,11 @@
       view.saveConfig();
       var message = conn.last_message();
       var params = message.Params;
-      assert.equal('ServiceSet', message.Request);
+      assert.equal('ServiceUpdate', message.Request);
       assert.equal('mysql', params.ServiceName);
-      assert.equal('new value', params.Config.option0);
+      assert.equal('new value', params.SettingsStrings.option0);
       // undefined because it should only set the changed values
-      assert.equal(undefined, params.Config.option1);
+      assert.equal(undefined, params.SettingsStrings.option1);
     });
 
     it('should reenable the "Update" button if RPC fails', function() {