Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Adds set_config to the ECS supported methods. #188

Merged
merged 2 commits into from
Mar 21, 2014
Merged

Adds set_config to the ECS supported methods. #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3ab05c
Added setConfig functionality to the ECS.
Mar 20, 2014
3959bed
Added tests for the next command functionality.
Mar 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
85 changes: 79 additions & 6 deletions app/utils/environment-change-set.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,23 @@ YUI.add('environment-change-set', function(Y) {
return key;
},

/**
Adds a new command to an existing record.

@method _addToRecrod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: _addToRecord.
Also, I am not sure how this works, see below.

@param {String} key The key to add the command to.
@param {Object} command The command that's to be executed lazily.
@return {String} The key to which the command was added.
*/
_addToRecord: function(key, command) {
command = this._wrapCallback(command);
var changeSet = this.changeSet[key];
var length = changeSet.commands.push(command);
// Add next function to execute to previous next() command.
changeSet.commands[length - 2].next = this._execute.bind(this, command);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you flesh this out a little bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you mean flesh out the comment more...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, it would be nice at this point to have a documentation on how you intend to design the changeSet data structure.
For example, does it allow to add a command to a nested key?

E.g. suppose you have the following lazy actions:

  1. add a new machine;
  2. create an LXC container inside the machine in 1;
  3. add a unit to the LXC in 2.
    The execution order is obviously important here, and we two nesting levels.

Here is another example:

  1. add a unit to a machine;
  2. remove that unit.
    This should result in a no-op when the queryset is committed. Will this kind of logic be handled here?

Another example (sorry, I write too much):

  1. create a new service;
  2. change settings for that service;
  3. change settings for that service again.
    In this case, I guess the more convenient thing to do is ignoring 2. With the current approach, it looks like we would execute both 2 and 3.

I'd like to knwo what do you think about the following.
What about having a single addCommand(changeSet, key, command) method allowing for nested keys? e.g. "machine-new-1.lxc-new-1.django/2"? If this is crazy, please ignore me.

Another question: it seems you designed commands to be a linked-list-like array. I am ok with that if we are sure we can easily traverse the tree and modify/remove existing keys. Right now it seems the only traversing enabled automatically executes the next command, and this feels dangerous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our hangout the underlying structure will change to a list which will then have convenience methods to parse and execute based on internal record references to their hierarchy.

return key;
},

/**
Wraps the last function parameter so that we can be notified when it's
called.
Expand All @@ -122,6 +139,10 @@ YUI.add('environment-change-set', function(Y) {
// under. In most cases this will be `env`.
var result = callback.apply(this, self._getArgs(arguments));
self.fire('taskComplete', command);
// Execute the next command.
if (command.next) {
command.next();
}
return result;
}
args[index] = _callbackWrapper;
Expand All @@ -132,25 +153,28 @@ YUI.add('environment-change-set', function(Y) {
Executes the passed in command on the environment.

@method _execute
@param {Object} command An object of the command to execute.
@param {Object} command The individual command object from the changeSet.
*/
_execute: function(command) {
var env = this.get('env');
env[command.method].apply(env, command.args);
},

/**
Executes all of the commands stored in the changeSet.
Starts the processing of all of the top level
commands stored in the changeSet.

@method commit
*/
commit: function() {
var changeSet = this.changeSet;
var commands;

Object.keys(changeSet).forEach(function(key) {
changeSet[key].commands.forEach(function(command) {
this._execute(command);
this.fire('commit', command);
}, this);
commands = changeSet[key].commands;
// Trigger the series to start executing.
this._execute(commands[0]);
this.fire('commit', commands);
}, this);
},

Expand All @@ -176,6 +200,30 @@ YUI.add('environment-change-set', function(Y) {
return this._createNewRecord('service', command);
},

/**
Creates a new entry in the queue for setting a services config.

Receives all the parameters it's public method 'set_config' was called
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Receives all the parameters its public method...

with with the exception of the ECS options oject.

@method _lazySetConfig
@param {Array} args The arguments to set the config with.
*/
_lazySetConfig: function(args) {
var serviceName = args[0];
var queued = this.changeSet[serviceName];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by the logic here. It seems to me that, when lazy deploying a service, a random key is generated for the changeSet. Here we use the service name as the key. I'd expect this key to never be found in the changeSet. Am I missing something. If so, please add a comment on that reguard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a known issue to be resolved by a shared service model between the ghost and service inspectors. This interaction will also be changed as per our hangout.

var command = {
method: 'set_config', // This needs to match the method name in env.
executed: false,
args: args
};
// If it's a queued service then we need to add to that service's record.
if (queued) {
return this._addToRecord(serviceName, command);
}
return this._createNewRecord('setConfig', command);
},

/* End private environment methods. */

/* Public environment methods. */
Expand All @@ -198,6 +246,31 @@ YUI.add('environment-change-set', function(Y) {
} else {
this._lazyDeploy(args);
}
},

/**
Calls the environments set_config method or creates a new set_config
record in the queue.

THe parameters match the parameters for the env deploy method.

@method setConfig
*/
setConfig: function(serviceName, config, data, serviceConfig, callback,
options) {
var env = this.get('env'),
args = this._getArgs(arguments);
if (options && options.immediate) {
// Need to check that the serviceName is a real service name and not
// a queued service id before allowing immediate or not.
if (this.changeSet[serviceName]) {
throw 'You cannot immediately setConfig on a queued service';
} else {
env.set_config.apply(env, args);
}
} else {
this._lazySetConfig(args);
}
}

/* End Public environment methods. */
Expand Down
4 changes: 3 additions & 1 deletion app/views/environment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ YUI.add('juju-view-environment', function(Y) {

var db = this.get('db'),
env = this.get('env'),
ecs = this.get('ecs'),
topo = this.topo,
charm = db.charms.getById(model.get('charm')),
inspector = {};
Expand All @@ -102,7 +103,7 @@ YUI.add('juju-view-environment', function(Y) {
db: db,
model: model,
env: env,
ecs: this.get('ecs'),
ecs: ecs,
environment: this,
charmModel: charm,
topo: topo,
Expand All @@ -113,6 +114,7 @@ YUI.add('juju-view-environment', function(Y) {
db: db,
model: model,
env: env,
ecs: ecs,
environment: this,
enableDatabinding: true,
topo: topo,
Expand Down
16 changes: 14 additions & 2 deletions app/views/viewlets/service-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ YUI.add('service-config-view', function(Y) {
*/
saveConfig: function() {
var inspector = this.viewletManager,
env = inspector.get('env'),
db = inspector.get('db'),
service = inspector.get('model'),
charmUrl = service.get('charm'),
Expand All @@ -162,7 +161,20 @@ YUI.add('service-config-view', function(Y) {
var errors = utils.validate(config, schema);

if (Y.Object.isEmpty(errors)) {
env.set_config(
var setConfigMethod;
if (window.flags.ecs) {
// When this flag is removed and the method is being called
// directly it doesn't need to be bound.
var ecs = this.get('ecs');
setConfigMethod = ecs.setConfig.bind(ecs);
} else {
var env = inspector.get('env');
setConfigMethod = env.set_config.bind(env);
}

setConfigMethod(
// When we have a ghost service model this id will have to be the
// changeSet id so that we know which service to modify.
service.get('id'),
config,
null,
Expand Down