Merge lp:~hatch/juju-gui/save-changed-1214087 into lp:juju-gui/experimental
- save-changed-1214087
- Merge into trunk
| Status: | Merged | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Merged at revision: | 1000 | ||||||||
| Proposed branch: | lp:~hatch/juju-gui/save-changed-1214087 | ||||||||
| Merge into: | lp:juju-gui/experimental | ||||||||
| Diff against target: |
826 lines (+268/-67) 17 files modified
app/store/env/fakebackend.js (+28/-2) app/store/env/go.js (+51/-6) app/store/env/python.js (+18/-5) app/views/ghost-inspector.js (+2/-0) app/views/inspector.js (+2/-0) app/views/service.js (+1/-0) app/views/utils.js (+38/-2) app/views/viewlets/service-ghost.js (+5/-0) test/assets/mysql-config.yaml (+1/-1) test/test_env_go.js (+9/-2) test/test_env_python.js (+9/-10) test/test_fakebackend.js (+21/-6) test/test_ghost_inspector.js (+3/-7) test/test_sandbox_go.js (+50/-12) test/test_sandbox_python.js (+22/-7) test/test_service_config_view.js (+4/-3) test/test_utils.js (+4/-4) |
||||||||
| To merge this branch: | bzr merge lp:~hatch/juju-gui/save-changed-1214087 | ||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Juju GUI Hackers | Pending | ||
|
Review via email:
|
|||
Commit message
Description of the change
Only sends the changed config options.
1) There was a bug with the ghost constraints code which
caused it to fail when deploying services on rapi.
2) The GUI now only sends configuration values to juju when
those values differ from the defaults.
| Jeff Pihach (hatch) wrote : | # |
| Francesco Banconi (frankban) wrote : | # |
Thanks for this branch Jeff. I think there is still some substantial
work/clean up to do here (see the comments below). The constraints
problem prevented me from deploying charms in juju-core most of the
times. In the go sandbox, I was able to deploy services and set
constraints, but a further look in the inspector showed the constraints
to be broken (e.g. a constraint named "1" with value "cpu-cores=4"). I
believe this is the same problem described below.
https:/
File app/views/
https:/
app/views/
proper format.
This is not the proper format for juju-core, e.g.:
D 130830 08:33:57 handlers:148] GET /ws (10.0.3.1) client -> juju:
{"Type"
[D 130830 08:33:57 handlers:164] GET /ws (10.0.3.1) juju -> client:
{"RequestId"
type constraints.
The deploy method of the GoEnvironment, and the set_constraints as well,
requires an object to be passed. The juju-core constraints.Value itself
is a struct, not a slice. The deploy method of the PyEnvironment does
not mention "constraints" in its docstring (and this should be fixed).
Since we eventually want to stop maintaining the pyjuju backend, I'd
suggest to generate the constraints as an object (like it is currently
done), and make the proper conversions only ub the case the environment
is python (or the python sandbox).
Does this make sense?
https:/
File app/views/
https:/
app/views/
This logic is used by both inspectors: I'd like to see this code
abstracted in a separate function, reused when required, and tested. A
comment explaining why we need implicit type conversion would be great.
- 997. By Jeff Pihach
-
fixed difference between backend constraint formats
| Jeff Pihach (hatch) wrote : | # |
Please take a look.
- 998. By Jeff Pihach
-
testing for go backend push
- 999. By Jeff Pihach
-
fixed constraints parsing
- 1000. By Jeff Pihach
-
tests and lint
| Jeff Pihach (hatch) wrote : | # |
Please take a look.
| Richard Harding (rharding) wrote : | # |
I think this moves the environment specific logic up to the view/app
layer. The inspector or other views shouldn't have to worry about
transposing the constraints. I think that should be moved down into the
environment and the utils helper made part of the env as well so that
the logic is localized since the env tests are what's testing the code
works anyway.
https:/
File app/store/env/go.js (right):
https:/
app/store/
'mem', 'arch'],
why not just set these as an object with key name, { type: } ?
https:/
File app/views/
https:/
app/views/
constraints to be
r*e*require
https:/
app/views/
this.options.
should the env.deploy function do the conversion instead of the
ghost-inspector? What if this logic were used elsewhere. If it's always
in the deploy then it won't need to be repeated.
https:/
File app/views/
https:/
app/views/
again, can set_config do the check for unchanged options? It seems it
would know the old values and have a list of the new ones to do the work
in the back end so that the inspector doesn't have to worry about back
end implementation.
https:/
File app/views/utils.js (right):
https:/
app/views/
function(config, charmOptions) {
This could be moved to the env itself as a helper used in there.
https:/
File test/test_
https:/
test/test_
arch: 'i386'};
?? did this cause a lint issue or something?
| Jeff Pihach (hatch) wrote : | # |
Thanks for the review - comments below and changes incoming.
I agree with your comments but where noted we can discuss in more detail
tomorrow.
https:/
File app/store/env/go.js (right):
https:/
app/store/
'mem', 'arch'],
I didn't want to change everywhere else in the application that accessed
the constraints objects from either back end and all of their associated
tests.
https:/
File app/views/
https:/
app/views/
constraints to be
On 2013/09/02 21:19:28, rharding wrote:
> r*e*require
Done.
https:/
app/views/
this.options.
That's a good idea, done.
https:/
File app/views/
https:/
app/views/
Lets discuss this tomorrow - I figured that this method may be called
directly and not have it's values run through this method.
https:/
File app/views/utils.js (right):
https:/
app/views/
function(config, charmOptions) {
As previously mentioned - lets discuss tomorrow.
https:/
File test/test_
https:/
test/test_
arch: 'i386'};
I don't think so, probably just an old habit.
- 1001. By Jeff Pihach
-
minor refactoring from review
- 1002. By Jeff Pihach
-
merged trunk
| Jeff Pihach (hatch) wrote : | # |
Please take a look.
- 1003. By Jeff Pihach
-
refactored set config to only send changed values
| Jeff Pihach (hatch) wrote : | # |
Please take a look.
| Richard Harding (rharding) wrote : | # |
LGTM thanks for getting this put together!
- 1004. By Jeff Pihach
-
normalized the constraint filtering in the go backend
- 1005. By Jeff Pihach
-
lint
| Jeff Pihach (hatch) wrote : | # |
Please take a look.
- 1006. By Jeff Pihach
-
fix incorrect typeof bug
| Jeff Pihach (hatch) wrote : | # |
Please take a look.
| Francesco Banconi (frankban) wrote : | # |
LGTM QA ok, thank you Jeff
| Jeff Pihach (hatch) wrote : | # |
*** Submitted:
Only sends the changed config options.
1) There was a bug with the ghost constraints code which
caused it to fail when deploying services on rapi.
2) The GUI now only sends configuration values to juju when
those values differ from the defaults.
R=frankban, rharding
CC=
https:/
Preview Diff
| 1 | === modified file 'app/store/env/fakebackend.js' |
| 2 | --- app/store/env/fakebackend.js 2013-09-03 10:02:52 +0000 |
| 3 | +++ app/store/env/fakebackend.js 2013-09-03 15:47:40 +0000 |
| 4 | @@ -374,14 +374,40 @@ |
| 5 | constraints = options.constraints; |
| 6 | } |
| 7 | |
| 8 | + // In order for the constraints to support the python back end this |
| 9 | + // needs to be an array, so we are converting it back to an object |
| 10 | + // here so that the GUI displays it properly. |
| 11 | + var constraintsMap = {}, vals; |
| 12 | + if (Y.Lang.isArray(constraints)) { |
| 13 | + constraints.forEach(function(cons) { |
| 14 | + vals = cons.split('='); |
| 15 | + constraintsMap[vals[0]] = vals[1]; |
| 16 | + }); |
| 17 | + } else { |
| 18 | + constraintsMap = constraints; |
| 19 | + } |
| 20 | + |
| 21 | var service = this.db.services.add({ |
| 22 | id: options.name, |
| 23 | name: options.name, |
| 24 | charm: charm.get('id'), |
| 25 | - constraints: constraints, |
| 26 | + constraints: constraintsMap, |
| 27 | exposed: false, |
| 28 | subordinate: charm.get('is_subordinate'), |
| 29 | - config: options.config |
| 30 | + // Because we only send the user changed options now |
| 31 | + // we need to mix those values in to the charm config |
| 32 | + // options when creating a new model. |
| 33 | + config: (function() { |
| 34 | + var charmOptions = charm.get('options'); |
| 35 | + var config = {}; |
| 36 | + if (!options.config) { options.config = {}; } |
| 37 | + Object.keys(charmOptions).forEach(function(key) { |
| 38 | + config[key] = |
| 39 | + options.config[key] || |
| 40 | + (charmOptions[key] ? charmOptions[key].default : undefined); |
| 41 | + }); |
| 42 | + return config; |
| 43 | + })() |
| 44 | }); |
| 45 | this.changes.services[service.get('id')] = [service, true]; |
| 46 | var response = this.addUnit(options.name, options.unitCount); |
| 47 | |
| 48 | === modified file 'app/store/env/go.js' |
| 49 | --- app/store/env/go.js 2013-08-28 19:21:23 +0000 |
| 50 | +++ app/store/env/go.js 2013-09-03 15:47:40 +0000 |
| 51 | @@ -28,6 +28,7 @@ |
| 52 | YUI.add('juju-env-go', function(Y) { |
| 53 | |
| 54 | var environments = Y.namespace('juju.environments'); |
| 55 | + var utils = Y.namespace('juju.views.utils'); |
| 56 | |
| 57 | var endpointToName = function(endpoint) { |
| 58 | return endpoint[0] + ':' + endpoint[1].name; |
| 59 | @@ -119,9 +120,28 @@ |
| 60 | |
| 61 | Y.extend(GoEnvironment, environments.BaseEnvironment, { |
| 62 | |
| 63 | + /** |
| 64 | + A list of the valid constraints for all providers. Required |
| 65 | + because we cannot request these constraints from Juju yet. |
| 66 | + |
| 67 | + @property genericConstraints |
| 68 | + @default ['cpu-power', 'cpu-cores', 'mem', 'arch'] |
| 69 | + @type {Array} |
| 70 | + */ |
| 71 | genericConstraints: ['cpu-power', 'cpu-cores', 'mem', 'arch'], |
| 72 | |
| 73 | /** |
| 74 | + A list of the constraints that need to be integers. We require |
| 75 | + this list because we cannot request the valid constraints from |
| 76 | + Juju. |
| 77 | + |
| 78 | + @property integerConstraints |
| 79 | + @default ['cpu-power', 'cpu-cores', 'mem'] |
| 80 | + @type {Array} |
| 81 | + */ |
| 82 | + integerConstraints: ['cpu-power', 'cpu-cores', 'mem'], |
| 83 | + |
| 84 | + /** |
| 85 | * Go environment constructor. |
| 86 | * |
| 87 | * @method initializer |
| 88 | @@ -264,6 +284,30 @@ |
| 89 | }, |
| 90 | |
| 91 | /** |
| 92 | + Utility method for filtering the constraint data and removing constraints |
| 93 | + which do not have valid values. |
| 94 | + |
| 95 | + @method filterConstraints |
| 96 | + @param {Object} constraints key:value pairs. |
| 97 | + @return an object of valid constraint values. |
| 98 | + */ |
| 99 | + filterConstraints: function(constraints) { |
| 100 | + // Some of the constraints have to be integers. |
| 101 | + this.integerConstraints.forEach(function(key) { |
| 102 | + constraints[key] = parseInt(constraints[key], 10) || undefined; |
| 103 | + }); |
| 104 | + // Remove all constraint options which don't have values |
| 105 | + // else the go back end has issues. |
| 106 | + Object.keys(constraints).forEach(function(key) { |
| 107 | + if (constraints[key] === undefined || |
| 108 | + (constraints[key].trim && constraints[key].trim() === '')) { |
| 109 | + delete constraints[key]; |
| 110 | + } |
| 111 | + }); |
| 112 | + return constraints; |
| 113 | + }, |
| 114 | + |
| 115 | + /** |
| 116 | * React to the results of sending a login message to the server. |
| 117 | * |
| 118 | * @method handleLogin |
| 119 | @@ -383,6 +427,8 @@ |
| 120 | console.error('Constraints need to be an object not a function'); |
| 121 | console.warn(constraints); |
| 122 | } |
| 123 | + |
| 124 | + constraints = this.filterConstraints(constraints); |
| 125 | } else { |
| 126 | constraints = {}; |
| 127 | } |
| 128 | @@ -852,14 +898,15 @@ |
| 129 | @param {String} data The YAML representation of the charm |
| 130 | configuration options. Only one of `config` and `data` should be |
| 131 | provided, though `data` takes precedence if it is given. |
| 132 | + @param {Object} serviceConfig the current configuration object |
| 133 | + of the service. |
| 134 | @param {Function} callback A callable that must be called once the |
| 135 | operation is performed. It will receive an object containing: |
| 136 | err - a string describing the problem (if an error occurred), |
| 137 | service_name - the name of the service. |
| 138 | @return {undefined} Sends a message to the server only. |
| 139 | */ |
| 140 | - set_config: function(serviceName, config, data, callback) { |
| 141 | - |
| 142 | + set_config: function(serviceName, config, data, serviceConfig, callback) { |
| 143 | if ((Y.Lang.isValue(config) && Y.Lang.isValue(data)) || |
| 144 | (!Y.Lang.isValue(config) && !Y.Lang.isValue(data))) { |
| 145 | throw 'Exactly one of config and data must be provided'; |
| 146 | @@ -878,6 +925,7 @@ |
| 147 | sendData.Request = 'ServiceSetYAML'; |
| 148 | sendData.Params.ConfigYAML = data; |
| 149 | } else { |
| 150 | + config = utils.removeUnchangedConfigOptions(config, serviceConfig); |
| 151 | sendData.Request = 'ServiceSet'; |
| 152 | sendData.Params.Config = stringifyObjectValues(config); |
| 153 | } |
| 154 | @@ -926,10 +974,7 @@ |
| 155 | intermediateCallback = Y.bind(this.handleSetConstraints, null, |
| 156 | callback, serviceName); |
| 157 | } |
| 158 | - // Some of the constraints have to be numbers. |
| 159 | - Y.Array.each(['cpu-cores', 'cpu-power', 'mem'], function(key) { |
| 160 | - constraints[key] = parseInt(constraints[key], 10) || undefined; |
| 161 | - }); |
| 162 | + constraints = this.filterConstraints(constraints); |
| 163 | sendData = { |
| 164 | Type: 'Client', |
| 165 | Request: 'SetServiceConstraints', |
| 166 | |
| 167 | === modified file 'app/store/env/python.js' |
| 168 | --- app/store/env/python.js 2013-08-26 16:52:38 +0000 |
| 169 | +++ app/store/env/python.js 2013-09-03 15:47:40 +0000 |
| 170 | @@ -28,6 +28,7 @@ |
| 171 | YUI.add('juju-env-python', function(Y) { |
| 172 | |
| 173 | var environments = Y.namespace('juju.environments'); |
| 174 | + var utils = Y.namespace('juju.views.utils'); |
| 175 | |
| 176 | var endpointToName = function(endpoint) { |
| 177 | return endpoint[0] + ':' + endpoint[1].name; |
| 178 | @@ -250,14 +251,21 @@ |
| 179 | configuration options. Only one of `config` and `config_raw` should be |
| 180 | provided, though `config_raw` takes precedence if it is given. |
| 181 | * @param {Integer} num_units The number of units to be deployed. |
| 182 | + * @param {Object} constraintMap The constraints object. |
| 183 | * @param {Function} callback A callable that must be called once the |
| 184 | operation is performed. |
| 185 | * @return {undefined} Sends a message to the server only. |
| 186 | */ |
| 187 | deploy: function(charm_url, service_name, config, config_raw, num_units, |
| 188 | - constraints, callback) { |
| 189 | - if (!constraints) { |
| 190 | - constraints = {}; |
| 191 | + constraintMap, callback) { |
| 192 | + |
| 193 | + if (!constraintMap) { constraintMap = {}; } |
| 194 | + // Format the constraints properly for the pyjuju backend |
| 195 | + var constraints = []; |
| 196 | + /* jshint -W089 */ |
| 197 | + // Tell jshint to ignore the lack of hasOwnProperty in forloops |
| 198 | + for (var constr in constraintMap) { |
| 199 | + constraints.push(constr + '=' + constraintMap[constr]); |
| 200 | } |
| 201 | |
| 202 | this._send_rpc( |
| 203 | @@ -391,20 +399,25 @@ |
| 204 | * @param {String} data The YAML representation of the charm |
| 205 | configuration options. Only one of `config` and `data` should be |
| 206 | provided, though `data` takes precedence if it is given. |
| 207 | + * @param {Object} serviceConfig the current configuration object |
| 208 | + of the service. |
| 209 | * @param {Function} callback A callable that must be called once the |
| 210 | operation is performed. |
| 211 | * @return {undefined} Sends a message to the server only. |
| 212 | */ |
| 213 | - set_config: function(service, config, data, callback) { |
| 214 | + set_config: function(service, config, data, serviceConfig, callback) { |
| 215 | if ((Y.Lang.isValue(config) && Y.Lang.isValue(data)) || |
| 216 | (!Y.Lang.isValue(config) && !Y.Lang.isValue(data))) { |
| 217 | throw 'Exactly one of config and data must be provided'; |
| 218 | } |
| 219 | + config = utils.removeUnchangedConfigOptions(config, serviceConfig); |
| 220 | + |
| 221 | this._send_rpc({ |
| 222 | op: 'set_config', |
| 223 | service_name: service, |
| 224 | config: config, |
| 225 | - data: data}, callback, true); |
| 226 | + data: data |
| 227 | + }, callback, true); |
| 228 | }, |
| 229 | |
| 230 | // The constraints that the backend understands. Used to generate forms. |
| 231 | |
| 232 | === modified file 'app/views/ghost-inspector.js' |
| 233 | --- app/views/ghost-inspector.js 2013-08-26 14:16:34 +0000 |
| 234 | +++ app/views/ghost-inspector.js 2013-09-03 15:47:40 +0000 |
| 235 | @@ -119,6 +119,8 @@ |
| 236 | } else { |
| 237 | config = utils.getElementsValuesMapping( |
| 238 | container, '.service-config .config-field'); |
| 239 | + config = utils.removeUnchangedConfigOptions( |
| 240 | + config, options.model.get('options')); |
| 241 | } |
| 242 | |
| 243 | // Deploy needs constraints in simple key:value object. |
| 244 | |
| 245 | === modified file 'app/views/inspector.js' |
| 246 | --- app/views/inspector.js 2013-08-29 17:10:19 +0000 |
| 247 | +++ app/views/inspector.js 2013-09-03 15:47:40 +0000 |
| 248 | @@ -734,11 +734,13 @@ |
| 249 | |
| 250 | var newVals = utils.getElementsValuesMapping(container, '.config-field'); |
| 251 | var errors = utils.validate(newVals, schema); |
| 252 | + |
| 253 | if (Y.Object.isEmpty(errors)) { |
| 254 | env.set_config( |
| 255 | service.get('id'), |
| 256 | newVals, |
| 257 | null, |
| 258 | + service.get('config'), |
| 259 | Y.bind(this._setConfigCallback, this, container) |
| 260 | ); |
| 261 | } else { |
| 262 | |
| 263 | === modified file 'app/views/service.js' |
| 264 | --- app/views/service.js 2013-08-06 04:35:06 +0000 |
| 265 | +++ app/views/service.js 2013-09-03 15:47:40 +0000 |
| 266 | @@ -630,6 +630,7 @@ |
| 267 | service.get('id'), |
| 268 | new_values, |
| 269 | null, |
| 270 | + service.get('config'), |
| 271 | Y.bind(this._setConfigCallback, this, container) |
| 272 | ); |
| 273 | |
| 274 | |
| 275 | === modified file 'app/views/utils.js' |
| 276 | --- app/views/utils.js 2013-08-30 20:02:41 +0000 |
| 277 | +++ app/views/utils.js 2013-09-03 15:47:40 +0000 |
| 278 | @@ -568,7 +568,7 @@ |
| 279 | 'cpu': {title: 'CPU', unit: 'GHz'}, |
| 280 | 'cpu-cores': {title: 'CPU Cores'}, |
| 281 | 'cpu-power': {title: 'CPU Power', unit: 'GHz'}, |
| 282 | - 'mem': {title: 'Memory', unit: 'GB'} |
| 283 | + 'mem': {title: 'Memory', unit: 'MB'} |
| 284 | }; |
| 285 | |
| 286 | /** |
| 287 | @@ -659,7 +659,43 @@ |
| 288 | }; |
| 289 | |
| 290 | /** |
| 291 | - Return a template-friendly array of settings |
| 292 | + Removes unchanged config options from a collection of config values and |
| 293 | + returns only those that are different from the supplied charm or service |
| 294 | + defaults at the time of parsing. |
| 295 | + |
| 296 | + @method removeUnchangedConfigOptions |
| 297 | + @param {Object} config is a reference to service config values in the GUI. |
| 298 | + @param {Object} options is a reference to the charm or |
| 299 | + service configuration options. |
| 300 | + @return {Object} An object containing the key/value pairs of config options. |
| 301 | + */ |
| 302 | + utils.removeUnchangedConfigOptions = function(config, options) { |
| 303 | + // This method is always called even if the config is provided by |
| 304 | + // a configuration file - in this case, return; |
| 305 | + if (!config) { return; } |
| 306 | + Object.keys(config).forEach(function(key) { |
| 307 | + // Remove config options which are not different from the charm defaults |
| 308 | + // Intentionally letting the browser do the type coersion. |
| 309 | + // The || check is to allow empty inputs to match an undefined default |
| 310 | + /* jshint -W116 */ |
| 311 | + if (Y.Lang.isObject(options[key])) { |
| 312 | + // If options is the charm config |
| 313 | + if (config[key] == (options[key].default || '')) { |
| 314 | + delete config[key]; |
| 315 | + } |
| 316 | + } else { |
| 317 | + // If options is a service config |
| 318 | + if (config[key] == (options[key] || '')) { |
| 319 | + delete config[key]; |
| 320 | + } |
| 321 | + } |
| 322 | + /* jshint +W116 */ |
| 323 | + }); |
| 324 | + return config; |
| 325 | + }; |
| 326 | + |
| 327 | + /** |
| 328 | + Return a template-friendly array of settings. |
| 329 | |
| 330 | Used for service-configuration.partial adding isBool, isNumeric metadata. |
| 331 | |
| 332 | |
| 333 | === modified file 'app/views/viewlets/service-ghost.js' |
| 334 | --- app/views/viewlets/service-ghost.js 2013-08-26 17:03:22 +0000 |
| 335 | +++ app/views/viewlets/service-ghost.js 2013-09-03 15:47:40 +0000 |
| 336 | @@ -41,6 +41,11 @@ |
| 337 | var newVal = (val['default'] === undefined) ? '' : val['default']; |
| 338 | node.set('value', newVal); |
| 339 | } |
| 340 | + }, |
| 341 | + 'constraints': { |
| 342 | + 'update': function(node, val) { |
| 343 | + node.set('value', val || ''); |
| 344 | + } |
| 345 | } |
| 346 | }, |
| 347 | 'render': function(model, viewletMgrAttrs) { |
| 348 | |
| 349 | === modified file 'test/assets/mysql-config.yaml' |
| 350 | --- test/assets/mysql-config.yaml 2012-10-14 16:23:33 +0000 |
| 351 | +++ test/assets/mysql-config.yaml 2013-09-03 15:47:40 +0000 |
| 352 | @@ -1,2 +1,2 @@ |
| 353 | - tuning-level: |
| 354 | + tuning: |
| 355 | super bad |
| 356 | |
| 357 | === modified file 'test/test_env_go.js' |
| 358 | --- test/test_env_go.js 2013-08-28 17:15:33 +0000 |
| 359 | +++ test/test_env_go.js 2013-09-03 15:47:40 +0000 |
| 360 | @@ -805,7 +805,14 @@ |
| 361 | }); |
| 362 | |
| 363 | it('can set a service config', function() { |
| 364 | - env.set_config('mysql', {'cfg-key': 'cfg-val'}); |
| 365 | + // This also tests that it only sends the changed values |
| 366 | + env.set_config('mysql', { |
| 367 | + 'cfg-key': 'cfg-val', |
| 368 | + 'unchanged': 'bar' |
| 369 | + }, null, { |
| 370 | + 'cfg-key': 'foo', |
| 371 | + 'unchanged': 'bar' |
| 372 | + }); |
| 373 | msg = conn.last_message(); |
| 374 | var expected = { |
| 375 | Type: 'Client', |
| 376 | @@ -841,7 +848,7 @@ |
| 377 | |
| 378 | it('handles failed set config', function() { |
| 379 | var err, service_name; |
| 380 | - env.set_config('yoursql', {}, null, function(evt) { |
| 381 | + env.set_config('yoursql', {}, null, {}, function(evt) { |
| 382 | err = evt.err; |
| 383 | service_name = evt.service_name; |
| 384 | }); |
| 385 | |
| 386 | === modified file 'test/test_env_python.js' |
| 387 | --- test/test_env_python.js 2013-08-27 16:24:00 +0000 |
| 388 | +++ test/test_env_python.js 2013-09-03 15:47:40 +0000 |
| 389 | @@ -71,14 +71,12 @@ |
| 390 | }); |
| 391 | |
| 392 | it('successfully deploys a service with constraints', function() { |
| 393 | - var constraints = { |
| 394 | - 'cpu': 1, |
| 395 | - 'mem': '512M', |
| 396 | - 'arch': 'i386' |
| 397 | - }; |
| 398 | + var constraints = { cpu: '1', mem: '512M', arch: 'i386'}; |
| 399 | env.deploy('precise/mysql', null, null, null, 1, constraints); |
| 400 | msg = conn.last_message(); |
| 401 | - assert.deepEqual(msg.constraints, constraints); |
| 402 | + // The python backend needs to format them differently than the go |
| 403 | + // backend to support rapi. |
| 404 | + assert.deepEqual(msg.constraints, [ 'cpu=1', 'mem=512M', 'arch=i386' ]); |
| 405 | }); |
| 406 | |
| 407 | it('can add a unit', function() { |
| 408 | @@ -202,7 +200,8 @@ |
| 409 | |
| 410 | it('can set a service config', function() { |
| 411 | var config = {'cfg-key': 'cfg-val'}; |
| 412 | - env.set_config('mysql', config); |
| 413 | + // This also tests that it only sends changed values. |
| 414 | + env.set_config('mysql', config, null, {}); |
| 415 | msg = conn.last_message(); |
| 416 | assert.equal(msg.op, 'set_config'); |
| 417 | assert.equal(msg.service_name, 'mysql'); |
| 418 | @@ -213,7 +212,7 @@ |
| 419 | /*jshint multistr:true */ |
| 420 | var data = 'tuning-level: \nexpert-mojo'; |
| 421 | /*jshint multistr:false */ |
| 422 | - env.set_config('mysql', null, data); |
| 423 | + env.set_config('mysql', null, data, null); |
| 424 | msg = conn.last_message(); |
| 425 | assert.equal(msg.op, 'set_config'); |
| 426 | assert.equal(msg.service_name, 'mysql'); |
| 427 | @@ -222,7 +221,7 @@ |
| 428 | |
| 429 | it('handles failed set config', function() { |
| 430 | var err, service_name; |
| 431 | - env.set_config('yoursql', {}, null, function(evt) { |
| 432 | + env.set_config('yoursql', {}, null, {}, function(evt) { |
| 433 | err = evt.err; |
| 434 | service_name = evt.service_name; |
| 435 | }); |
| 436 | @@ -479,7 +478,7 @@ |
| 437 | }); |
| 438 | |
| 439 | it('denies changes to config options if the GUI is read-only', function() { |
| 440 | - assertOperationDenied('set_config', ['haproxy', {}, null]); |
| 441 | + assertOperationDenied('set_config', ['haproxy', {}, null, {}]); |
| 442 | }); |
| 443 | |
| 444 | it('denies changing constraints if the GUI is read-only', function() { |
| 445 | |
| 446 | === modified file 'test/test_fakebackend.js' |
| 447 | --- test/test_fakebackend.js 2013-09-02 16:37:49 +0000 |
| 448 | +++ test/test_fakebackend.js 2013-09-03 15:47:40 +0000 |
| 449 | @@ -121,7 +121,12 @@ |
| 450 | annotations: {}, |
| 451 | aggregated_status: undefined, |
| 452 | charm: 'cs:precise/wordpress-15', |
| 453 | - config: undefined, |
| 454 | + config: { |
| 455 | + debug: 'no', |
| 456 | + engine: 'nginx', |
| 457 | + tuning: 'single', |
| 458 | + 'wp-content': '' |
| 459 | + }, |
| 460 | constraints: {}, |
| 461 | constraintsStr: undefined, |
| 462 | destroyed: false, |
| 463 | @@ -216,8 +221,13 @@ |
| 464 | |
| 465 | it('accepts a config.', function() { |
| 466 | fakebackend.deploy( |
| 467 | - 'cs:precise/wordpress-15', callback, {config: {funny: 'business'}}); |
| 468 | - assert.deepEqual(result.service.get('config'), {funny: 'business'}); |
| 469 | + 'cs:precise/wordpress-15', callback, {config: {engine: 'apache'}}); |
| 470 | + assert.deepEqual(result.service.get('config'), { |
| 471 | + debug: 'no', |
| 472 | + engine: 'apache', |
| 473 | + tuning: 'single', |
| 474 | + 'wp-content': '' |
| 475 | + }); |
| 476 | }); |
| 477 | |
| 478 | it('deploys multiple units.', function() { |
| 479 | @@ -247,8 +257,13 @@ |
| 480 | fakebackend.deploy( |
| 481 | 'cs:precise/wordpress-15', |
| 482 | callback, |
| 483 | - {config: {funny: 'business'}, configYAML: 'funny: girl'}); |
| 484 | - assert.deepEqual(result.service.get('config'), {funny: 'girl'}); |
| 485 | + {config: {funny: 'business'}, configYAML: 'engine: apache'}); |
| 486 | + assert.deepEqual(result.service.get('config'), { |
| 487 | + debug: 'no', |
| 488 | + engine: 'apache', |
| 489 | + tuning: 'single', |
| 490 | + 'wp-content': '' |
| 491 | + }); |
| 492 | }); |
| 493 | |
| 494 | it('rejects a non-string configYAML', function() { |
| 495 | @@ -264,7 +279,7 @@ |
| 496 | {configYAML: |
| 497 | Y.io('assets/mysql-config.yaml', {sync: true}).responseText}); |
| 498 | assert.isObject(result.service.get('config')); |
| 499 | - assert.equal(result.service.get('config')['tuning-level'], 'super bad'); |
| 500 | + assert.equal(result.service.get('config').tuning, 'super bad'); |
| 501 | }); |
| 502 | |
| 503 | it('rejects unparseable YAML config string.', function() { |
| 504 | |
| 505 | === modified file 'test/test_ghost_inspector.js' |
| 506 | --- test/test_ghost_inspector.js 2013-08-27 16:30:39 +0000 |
| 507 | +++ test/test_ghost_inspector.js 2013-09-03 15:47:40 +0000 |
| 508 | @@ -139,11 +139,7 @@ |
| 509 | var message = env.ws.last_message(); |
| 510 | var params = message.Params; |
| 511 | var config = { |
| 512 | - admins: '', |
| 513 | - debug: 'false', |
| 514 | - logo: '', |
| 515 | - name: 'foo', |
| 516 | - skin: 'vector' |
| 517 | + name: 'foo' |
| 518 | }; |
| 519 | assert.equal('ServiceDeploy', message.Request); |
| 520 | assert.equal('mediawiki', params.ServiceName); |
| 521 | @@ -189,7 +185,7 @@ |
| 522 | vmContainer.one('.viewlet-manager-footer button.confirm').simulate('click'); |
| 523 | |
| 524 | var message = env.ws.last_message(); |
| 525 | - assert.equal(message.constraints.cpu, '2'); |
| 526 | + assert.deepEqual(message.constraints, ['cpu=2', 'mem=', 'arch=']); |
| 527 | }); |
| 528 | |
| 529 | it('deploys with constraints in go env', function() { |
| 530 | @@ -204,7 +200,7 @@ |
| 531 | vmContainer.one('.viewlet-manager-footer button.confirm').simulate('click'); |
| 532 | |
| 533 | var message = env.ws.last_message(); |
| 534 | - assert.equal(message.Params.Constraints['cpu-power'], '2'); |
| 535 | + assert.deepEqual(message.Params.Constraints, { 'cpu-power': 2 }); |
| 536 | }); |
| 537 | |
| 538 | it('disables and resets input fields when \'use default config\' is active', |
| 539 | |
| 540 | === modified file 'test/test_sandbox_go.js' |
| 541 | --- test/test_sandbox_go.js 2013-09-02 16:42:27 +0000 |
| 542 | +++ test/test_sandbox_go.js 2013-09-03 15:47:40 +0000 |
| 543 | @@ -212,6 +212,12 @@ |
| 544 | 'Exposed': false, |
| 545 | 'CharmURL': 'cs:precise/wordpress-15', |
| 546 | 'Life': 'alive', |
| 547 | + 'Config': { |
| 548 | + debug: 'no', |
| 549 | + engine: 'nginx', |
| 550 | + tuning: 'single', |
| 551 | + 'wp-content': '' |
| 552 | + }, |
| 553 | 'Constraints': {} |
| 554 | }], |
| 555 | ['machine', 'change', {'Status': 'running'}], |
| 556 | @@ -238,7 +244,7 @@ |
| 557 | Params: { |
| 558 | CharmUrl: 'cs:precise/wordpress-15', |
| 559 | ServiceName: 'kumquat', |
| 560 | - ConfigYAML: 'funny: business', |
| 561 | + ConfigYAML: 'engine: apache', |
| 562 | NumUnits: 2 |
| 563 | }, |
| 564 | RequestId: 42 |
| 565 | @@ -252,7 +258,12 @@ |
| 566 | var service = state.db.services.getById('kumquat'); |
| 567 | assert.isObject(service); |
| 568 | assert.equal(service.get('charm'), 'cs:precise/wordpress-15'); |
| 569 | - assert.deepEqual(service.get('config'), {funny: 'business'}); |
| 570 | + assert.deepEqual(service.get('config'), { |
| 571 | + debug: 'no', |
| 572 | + engine: 'apache', |
| 573 | + tuning: 'single', |
| 574 | + 'wp-content': '' |
| 575 | + }); |
| 576 | var units = state.db.units.get_units_for_service(service); |
| 577 | assert.lengthOf(units, 2); |
| 578 | done(); |
| 579 | @@ -269,12 +280,17 @@ |
| 580 | assert.equal(result.charm_url, 'cs:precise/wordpress-15'); |
| 581 | var service = state.db.services.getById('kumquat'); |
| 582 | assert.equal(service.get('charm'), 'cs:precise/wordpress-15'); |
| 583 | - assert.deepEqual(service.get('config'), {llama: 'pajama'}); |
| 584 | + assert.deepEqual(service.get('config'), { |
| 585 | + debug: 'no', |
| 586 | + engine: 'apache', |
| 587 | + tuning: 'single', |
| 588 | + 'wp-content': '' |
| 589 | + }); |
| 590 | }; |
| 591 | env.deploy( |
| 592 | 'cs:precise/wordpress-15', |
| 593 | 'kumquat', |
| 594 | - {llama: 'pajama'}, |
| 595 | + {engine: 'apache'}, |
| 596 | null, |
| 597 | 1, |
| 598 | null, |
| 599 | @@ -440,7 +456,7 @@ |
| 600 | Request: 'ServiceSet', |
| 601 | Params: { |
| 602 | ServiceName: 'wordpress', |
| 603 | - Config: { llama: 'pajama' } |
| 604 | + Config: { engine: 'apache' } |
| 605 | }, |
| 606 | RequestId: 42 |
| 607 | }; |
| 608 | @@ -448,7 +464,12 @@ |
| 609 | var receivedData = Y.JSON.parse(received.data); |
| 610 | assert.isUndefined(receivedData.Error); |
| 611 | var service = state.db.services.getById('wordpress'); |
| 612 | - assert.deepEqual(service.get('config'), { llama: 'pajama' }); |
| 613 | + assert.deepEqual(service.get('config'), { |
| 614 | + debug: 'no', |
| 615 | + engine: 'apache', |
| 616 | + tuning: 'single', |
| 617 | + 'wp-content': '' |
| 618 | + }); |
| 619 | done(); |
| 620 | }; |
| 621 | client.open(); |
| 622 | @@ -462,10 +483,17 @@ |
| 623 | var callback = function(result) { |
| 624 | assert.isUndefined(result.err); |
| 625 | var service = state.db.services.getById('wordpress'); |
| 626 | - assert.deepEqual(service.get('config'), { llama: 'pajama' }); |
| 627 | + assert.deepEqual(service.get('config'), { |
| 628 | + debug: 'no', |
| 629 | + engine: 'apache', |
| 630 | + tuning: 'single', |
| 631 | + 'wp-content': '' |
| 632 | + }); |
| 633 | done(); |
| 634 | }; |
| 635 | - env.set_config('wordpress', {llama: 'pajama'}, undefined, callback); |
| 636 | + env.set_config('wordpress', { |
| 637 | + engine: 'apache' |
| 638 | + }, undefined, {}, callback); |
| 639 | }); |
| 640 | }); |
| 641 | |
| 642 | @@ -476,7 +504,7 @@ |
| 643 | Request: 'ServiceSetYAML', |
| 644 | Params: { |
| 645 | ServiceName: 'wordpress', |
| 646 | - ConfigYAML: 'wordpress:\n llama: pajama' |
| 647 | + ConfigYAML: 'wordpress:\n engine: apache' |
| 648 | }, |
| 649 | RequestId: 42 |
| 650 | }; |
| 651 | @@ -484,7 +512,12 @@ |
| 652 | var receivedData = Y.JSON.parse(received.data); |
| 653 | assert.isUndefined(receivedData.Error); |
| 654 | var service = state.db.services.getById('wordpress'); |
| 655 | - assert.deepEqual(service.get('config'), { llama: 'pajama' }); |
| 656 | + assert.deepEqual(service.get('config'), { |
| 657 | + debug: 'no', |
| 658 | + engine: 'apache', |
| 659 | + tuning: 'single', |
| 660 | + 'wp-content': '' |
| 661 | + }); |
| 662 | done(); |
| 663 | }; |
| 664 | client.open(); |
| 665 | @@ -498,11 +531,16 @@ |
| 666 | var callback = function(result) { |
| 667 | assert.isUndefined(result.err); |
| 668 | var service = state.db.services.getById('wordpress'); |
| 669 | - assert.deepEqual(service.get('config'), { llama: 'pajama' }); |
| 670 | + assert.deepEqual(service.get('config'), { |
| 671 | + debug: 'no', |
| 672 | + engine: 'apache', |
| 673 | + tuning: 'single', |
| 674 | + 'wp-content': '' |
| 675 | + }); |
| 676 | done(); |
| 677 | }; |
| 678 | env.set_config('wordpress', undefined, |
| 679 | - 'wordpress:\n llama: pajama', callback); |
| 680 | + 'wordpress:\n engine: apache', {}, callback); |
| 681 | }); |
| 682 | }); |
| 683 | |
| 684 | |
| 685 | === modified file 'test/test_sandbox_python.js' |
| 686 | --- test/test_sandbox_python.js 2013-08-22 19:04:38 +0000 |
| 687 | +++ test/test_sandbox_python.js 2013-09-03 15:47:40 +0000 |
| 688 | @@ -341,7 +341,7 @@ |
| 689 | op: 'deploy', |
| 690 | charm_url: 'cs:precise/wordpress-15', |
| 691 | service_name: 'kumquat', |
| 692 | - config_raw: 'funny: business', |
| 693 | + config_raw: 'engine: apache', |
| 694 | num_units: 2, |
| 695 | request_id: 42 |
| 696 | }; |
| 697 | @@ -358,7 +358,12 @@ |
| 698 | var service = state.db.services.getById('kumquat'); |
| 699 | assert.isObject(service); |
| 700 | assert.equal(service.get('charm'), 'cs:precise/wordpress-15'); |
| 701 | - assert.deepEqual(service.get('config'), {funny: 'business'}); |
| 702 | + assert.deepEqual(service.get('config'), { |
| 703 | + debug: 'no', |
| 704 | + engine: 'apache', |
| 705 | + tuning: 'single', |
| 706 | + 'wp-content': '' |
| 707 | + }); |
| 708 | var units = state.db.units.get_units_for_service(service); |
| 709 | assert.lengthOf(units, 2); |
| 710 | done(); |
| 711 | @@ -376,13 +381,18 @@ |
| 712 | assert.equal(result.charm_url, 'cs:precise/wordpress-15'); |
| 713 | var service = state.db.services.getById('kumquat'); |
| 714 | assert.equal(service.get('charm'), 'cs:precise/wordpress-15'); |
| 715 | - assert.deepEqual(service.get('config'), {llama: 'pajama'}); |
| 716 | + assert.deepEqual(service.get('config'), { |
| 717 | + debug: 'no', |
| 718 | + engine: 'apache', |
| 719 | + tuning: 'single', |
| 720 | + 'wp-content': '' |
| 721 | + }); |
| 722 | done(); |
| 723 | }; |
| 724 | env.deploy( |
| 725 | 'cs:precise/wordpress-15', |
| 726 | 'kumquat', |
| 727 | - {llama: 'pajama'}, |
| 728 | + {engine: 'apache'}, |
| 729 | null, |
| 730 | 1, |
| 731 | null, |
| 732 | @@ -799,14 +809,19 @@ |
| 733 | var op = { |
| 734 | op: 'set_config', |
| 735 | service_name: 'wordpress', |
| 736 | - config: {'blog-title': 'Inimical'}, |
| 737 | + config: {'engine': 'apache'}, |
| 738 | request_id: 99 |
| 739 | }; |
| 740 | client.onmessage = function(received) { |
| 741 | var parsed = Y.JSON.parse(received.data); |
| 742 | - assert.deepEqual(parsed.result, {'blog-title': 'Inimical'}); |
| 743 | + assert.deepEqual(parsed.result, { |
| 744 | + debug: 'no', |
| 745 | + engine: 'apache', |
| 746 | + tuning: 'single', |
| 747 | + 'wp-content': '' |
| 748 | + }); |
| 749 | var service = state.db.services.getById('wordpress'); |
| 750 | - assert.equal(service.get('config')['blog-title'], 'Inimical'); |
| 751 | + assert.equal(service.get('config').engine, 'apache'); |
| 752 | // Error should be undefined. |
| 753 | done(parsed.error); |
| 754 | }; |
| 755 | |
| 756 | === modified file 'test/test_service_config_view.js' |
| 757 | --- test/test_service_config_view.js 2013-08-27 16:24:00 +0000 |
| 758 | +++ test/test_service_config_view.js 2013-09-03 15:47:40 +0000 |
| 759 | @@ -160,7 +160,8 @@ |
| 760 | assert.equal('ServiceSet', message.Request); |
| 761 | assert.equal('mysql', params.ServiceName); |
| 762 | assert.equal('new value', params.Config.option0); |
| 763 | - assert.equal('true', params.Config.option1); |
| 764 | + // undefined because it should only set the changed values |
| 765 | + assert.equal(undefined, params.Config.option1); |
| 766 | }); |
| 767 | |
| 768 | it('should reenable the "Update" button if RPC fails', function() { |
| 769 | @@ -168,7 +169,7 @@ |
| 770 | var save_button = container.one('#save-service-config'); |
| 771 | save_button.get('disabled').should.equal(shouldBe); |
| 772 | }; |
| 773 | - env.set_config = function(service, config, data, callback) { |
| 774 | + env.set_config = function(service, config, data, serviceCfg, callback) { |
| 775 | assertButtonDisabled(true); |
| 776 | callback(ev); |
| 777 | }; |
| 778 | @@ -226,7 +227,7 @@ |
| 779 | // Mock function |
| 780 | // view.saveConfig() calls it as part of its internal |
| 781 | // "success" callback |
| 782 | - env.set_config = function(service, config, data, callback) { |
| 783 | + env.set_config = function(service, config, data, serviceCfg, callback) { |
| 784 | callback(ev); |
| 785 | }; |
| 786 | var ev = {err: false}, |
| 787 | |
| 788 | === modified file 'test/test_utils.js' |
| 789 | --- test/test_utils.js 2013-08-19 20:51:17 +0000 |
| 790 | +++ test/test_utils.js 2013-09-03 15:47:40 +0000 |
| 791 | @@ -140,7 +140,7 @@ |
| 792 | before(function() { |
| 793 | getConstraints = utils.getConstraints; |
| 794 | serviceConstraints = {arch: 'lcars', cpu: 'quantum', mem: 'teraflop'}; |
| 795 | - customConstraints = {foo: 'bar', arch: 'amd64', mem: '1GB'}; |
| 796 | + customConstraints = {foo: 'bar', arch: 'amd64', mem: '1MB'}; |
| 797 | genericConstraints = ['cpu', 'mem', 'arch']; |
| 798 | readOnlyConstraints = utils.readOnlyConstraints; |
| 799 | constraintDescriptions = utils.constraintDescriptions; |
| 800 | @@ -149,7 +149,7 @@ |
| 801 | it('correctly returns constraints for a service', function() { |
| 802 | var expected = [ |
| 803 | {name: 'cpu', value: 'quantum', title: 'CPU', unit: 'GHz'}, |
| 804 | - {name: 'mem', value: 'teraflop', title: 'Memory', unit: 'GB'}, |
| 805 | + {name: 'mem', value: 'teraflop', title: 'Memory', unit: 'MB'}, |
| 806 | {name: 'arch', value: 'lcars', title: 'Architecture'} |
| 807 | ]; |
| 808 | var obtained = getConstraints(serviceConstraints, genericConstraints); |
| 809 | @@ -161,7 +161,7 @@ |
| 810 | it('handles missing service constraints', function() { |
| 811 | var expected = [ |
| 812 | {name: 'cpu', value: '', title: 'CPU', unit: 'GHz'}, |
| 813 | - {name: 'mem', value: '', title: 'Memory', unit: 'GB'}, |
| 814 | + {name: 'mem', value: '', title: 'Memory', unit: 'MB'}, |
| 815 | {name: 'arch', value: '', title: 'Architecture'} |
| 816 | ]; |
| 817 | var obtained = getConstraints({}, genericConstraints); |
| 818 | @@ -171,7 +171,7 @@ |
| 819 | it('includes unexpected service constraints', function() { |
| 820 | var expected = [ |
| 821 | {name: 'cpu', value: '', title: 'CPU', unit: 'GHz'}, |
| 822 | - {name: 'mem', value: '1GB', title: 'Memory', unit: 'GB'}, |
| 823 | + {name: 'mem', value: '1MB', title: 'Memory', unit: 'MB'}, |
| 824 | {name: 'arch', value: 'amd64', title: 'Architecture'}, |
| 825 | {name: 'foo', value: 'bar', title: 'foo'} |
| 826 | ]; |
Reviewers: mp+183048_
Message:
Please take a look.
Description:
Only sends the changed config options.
1) There was a bug with the ghost constraints code which
caused it to fail when deploying services on rapi.
2) The GUI now only sends configuration values to juju when
those values differ from the defaults.
To QA: Deploy charms on sandbox, rapi, juju-core. Modifying and
not modifying the default constraints and configuration.
Then open the inspector and check to make sure the settings
are appropriate and that the constraints page shows the
proper constraints.
https:/
(do not edit description out of merge proposal)
Please review this at https:/
Affected files:
A [revision details]
M app/views/
M app/views/
M app/views/
M test/test_
Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>
Index: test/test_
=== modified file 'test/test_
--- test/test_
+++ test/test_
@@ -139,11 +139,7 @@
var message = env.ws.
var params = message.Params;
var config = {
- admins: '',
- debug: 'false',
- logo: '',
- name: 'foo',
- skin: 'vector'
+ name: 'foo'
};
assert.
assert.
@@ -189,7 +185,7 @@
vmContain
button.
var message = env.ws.
- assert.
+ assert.
});
it('deploys with constraints in go env', function() {
@@ -204,7 +200,8 @@
vmContain
button.
var message = env.ws.
- assert.
+ assert.
+ ['cpu-power=2', 'cpu-cores=', 'mem=', 'arch=']);
});
it('disables and resets input fields when \'use default config\' is
active',
Index: app/views/
=== modified file 'app/views/
--- app/views/
+++ app/views/
@@ -121,9 +121,30 @@
}
+ var charmOptions = options.
+ /*jshint -W089 */
+ // Tells jshint to ignore the lack of hasOwnProperty in forloops
+ for (var cfgOption in config) {
+ // Remove config options which are not different from the charm ...