Fix annotations on go backend

Fix annotations on go backend

Fix annotation roundtrips with go backend
      Splits how we prepare deltas for py/go juju fakebackends
      Change test expectations to merge updateAnnotation calls
  
  Fix import of constraints
  Fix export of undefined/default values
  
  Adjust simulator unit count rate of change.

https://code.launchpad.net/~bcsaller/juju-gui/restore-bundle-annotations/+merge/190298

(do not edit description out of merge proposal)

[bcsaller, 2013-10-10 05:58:21]

Please take a look.

[benjamin.saller, 2013-10-10 06:03:05]

Some reviewer notes

https://codereview.appspot.com/14485046/diff/1/app/models/handlers.js
File app/models/handlers.js (right):

https://codereview.appspot.com/14485046/diff/1/app/models/handlers.js#newcode279
app/models/handlers.js:279: models.setAnnotations(instance, change.Annotations,
true);
use util method

https://codereview.appspot.com/14485046/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14485046/diff/1/app/models/models.js#newcode292
app/models/models.js:292: }
model is more flexible in what it accepts

a=b,c=d style as well as object

https://codereview.appspot.com/14485046/diff/1/app/models/models.js#newcode299
app/models/models.js:299: result.push(k + '=' + v);
for export mostly, skip values that are undefined

https://codereview.appspot.com/14485046/diff/1/app/models/models.js#newcode1175
app/models/models.js:1175: var charmOptions = charm.get('options');
old vs new models, this didn't follow the models changing default options
location

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (left):

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#oldco...
app/store/env/sandbox.js:239: /**
This has moved to be a method on both py and go backends with slightly different
behavior.

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1363: var result = state.updateAnnotations(entityId,
data.Params.Pairs);
Add support for more entity types.

https://codereview.appspot.com/14485046/diff/1/test/data/wp-deployer.yaml
File test/data/wp-deployer.yaml (right):

https://codereview.appspot.com/14485046/diff/1/test/data/wp-deployer.yaml#new...
test/data/wp-deployer.yaml:28: constraints: 'cpu-power=2,cpu-cores=4'
for testing

[gary.poster, 2013-10-10 11:50:11]

Very nice!  Thank you.  LGTM with two trivials.  The comma simply shouldn't be
there AFAIK.  On reflection, I think I am wrong about the other issue: we should
not convert empty strings to undefineds.  However, I welcome your thoughts
there.

Thanks!

Gary

https://codereview.appspot.com/14485046/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14485046/diff/1/app/models/models.js#newcode287
app/models/models.js:287: output[kv[0]] = kv[1];
should you convert empty strings to undefineds?

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (left):

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#oldco...
app/store/env/sandbox.js:239: /**
On 2013/10/10 06:03:05, benjamin.saller wrote:
> This has moved to be a method on both py and go backends with slightly
different
> behavior.

Good approach.

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:346: _prepareDelta: function() {
AFAICT this one is not changed from the original, yeah?

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:853: Config: 'config',
I'm surprised, and even a bit concerned, this made it past the linter.  That
comma shouldn't be here, should it?

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:913: return entity.id;
Yeah, I wondered about this when I saw it...

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1047: var annotations = this._prepareAnnotations();
To all of the associated fix, here & above: yay!

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1363: var result = state.updateAnnotations(entityId,
data.Params.Pairs);
On 2013/10/10 06:03:05, benjamin.saller wrote:
> Add support for more entity types.

And also yay.

[benjamin.saller, 2013-10-10 15:43:11]

Thanks for the review.

https://codereview.appspot.com/14485046/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14485046/diff/1/app/models/models.js#newcode287
app/models/models.js:287: output[kv[0]] = kv[1];
On 2013/10/10 11:50:11, gary.poster wrote:
> should you convert empty strings to undefineds?

Not sure that we need to, in general I've been reading the go code to try to
figure out if we have the proper semantics in the places we think we do, this is
one example and setAnnotations really being what we thought of as
updateAnnotations (merge) was another. 

I consider these a whole in the interface between the projects and something to
keep an eye on.

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/14485046/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:346: _prepareDelta: function() {
On 2013/10/10 11:50:11, gary.poster wrote:
> AFAICT this one is not changed from the original, yeah?

Yeah, same same

[jeff.pihach, 2013-10-10 16:50:13]

LGTM with some trivial comment cleanup/additions thanks for this fix!

https://codereview.appspot.com/14485046/diff/10001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/14485046/diff/10001/app/models/models.js#newco...
app/models/models.js:282: 'setter': function(value) {
I'd like a quick comment here explaining what the purpose of this is for others
coming in later.

https://codereview.appspot.com/14485046/diff/10001/app/store/env/sandbox.js
File app/store/env/sandbox.js (right):

https://codereview.appspot.com/14485046/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:949: Prepare a delta of events to send to the client
since the last time they
Could you update the comments here now that we have two of these methods.

https://codereview.appspot.com/14485046/diff/10001/app/store/env/sandbox.js#n...
app/store/env/sandbox.js:974: // The unit changeType is actually "serviceUnit"
in the Python
here too

[bcsaller, 2013-10-10 17:07:29]

*** Submitted:

Fix annotations on go backend

Fix annotation roundtrips with go backend
      Splits how we prepare deltas for py/go juju fakebackends
      Change test expectations to merge updateAnnotation calls
  
  Fix import of constraints
  Fix export of undefined/default values
  
  Adjust simulator unit count rate of change.

R=benjamin.saller, gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/14485046