Merge lp://staging/~gary/juju-gui/sandboxEnv-2 into lp://staging/juju-gui/experimental

Reviewers: mp+152805_code.launchpad.net,

Message:
Please take a look.

Description:
Complete deploy functionality and tests.

This combines three discrete improvements, each with their own commit.
I combined them because they were kind of small in isolation. Maybe
that was silly or not helpful; I'm not sure how I feel about it yet. If
you have an opinion about the combination I'd be happy to hear it.

revno: 423
   add machines for units

revno: 424
   add support for passing a YAML string for service configuration, as
would be passed by file parsing

revno: 425
   add a nextChanges function and tests for it in isolation and with
deploy and addUnit.

Note that rev 424 uses js-yaml 2.0.3, which seems to be a lot better
than previous incarnations in my small tests. I decided it was worth
including. As I said in the .min.js file, I committed this to the tree
rather than using npm because the file we needed was not in npm.

rev 425, with the nextChanges, is the most "sketchy." We'll have to see
how that fares when we actually implement the wire protocol, but I think
it should work.

For now I'm proceeding with using "FakeBackend" as the name, despite
reservations from myself, Benji, and Ben. I want to move forward with
the code. That said, I'd be fine with changing this to "SandboxEnv."
What do you think?

Next steps in my plan are to write a first cut of the Python wire
protocol for the existing APIs, and to write a first cut of the Go
protocol. After that we should be able to just add in remaining
commands in the backend and the wire protocols. The only particularly
interesting one that I know of will be getEndpoints.

https://code.launchpad.net/~gary/juju-gui/sandboxEnv-2/+merge/152805

(do not edit description out of merge proposal)

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

Affected files:
   M .jshintrc
   M Makefile
   A [revision details]
   A app/assets/javascripts/js-yaml.min.js
   M app/modules-debug.js
   M app/store/env/fakebackend.js
   M bin/merge-files
   M test/test_fakebackend.js

LGTM

https://codereview.appspot.com/7741043/

LGTM modulo a couple of changes.

Before we'd not done any YAML parsing in JS due to Kapil's assertion
that there were no libraries that were acceptable. Is the one you've
chosen suitable? Can it be used in the parsing of config files when the
user uploads one? If so, that would make passing YAML obsolete and
would give a better user experience.

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

https://codereview.appspot.com/7741043/diff/1/app/store/env/fakebackend.js#newcode272
app/store/env/fakebackend.js:272: }
Originally improv accepted both and just gave precedence to configYAML.
The go implementation does the same so it would be nice for this
implementation to be consistent.

https://codereview.appspot.com/7741043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/7741043/diff/1/test/test_fakebackend.js#newcode200
test/test_fakebackend.js:200:
This test will need to be fixed if you make the previously suggested
change.

https://codereview.appspot.com/7741043/

On 03/12/2013 08:59 AM, Brad Crittenden wrote:
> LGTM modulo a couple of changes.
>
> Before we'd not done any YAML parsing in JS due to Kapil's assertion
> that there were no libraries that were acceptable. Is the one you've
> chosen suitable?

I think it is. As I mentioned in my cover letter, this is a new 2.x
rewrite of the recommended JS yaml parser. I believe it has been
released since Kapil's last evaluation. It parsed the juju-gui yaml
docs I had with flying colors, and the example the project gives
(http://nodeca.github.com/js-yaml/) looks good too.

> Can it be used in the parsing of config files when the
> user uploads one?

Yes, I think it could be.

> If so, that would make passing YAML obsolete and
> would give a better user experience.

Definitely agreed. However, I think we can use what I've done here as a
way to get our toes wet first. If this works out well, then we ought to
plan the change.

> https://codereview.appspot.com/7741043/diff/1/app/store/env/fakebackend.js
> File app/store/env/fakebackend.js (right):
>
> https://codereview.appspot.com/7741043/diff/1/app/store/env/fakebackend.js#newcode272
> app/store/env/fakebackend.js:272: }
> Originally improv accepted both and just gave precedence to configYAML.
> The go implementation does the same so it would be nice for this
> implementation to be consistent.

Ah, thank you. I didn't know that the Go version behaved identically,
but you would. :-) Thanks, I changed this.

>
> https://codereview.appspot.com/7741043/diff/1/test/test_fakebackend.js
> File test/test_fakebackend.js (right):
>
> https://codereview.appspot.com/7741043/diff/1/test/test_fakebackend.js#newcode200
> test/test_fakebackend.js:200:
> This test will need to be fixed if you make the previously suggested
> change.

Done.

Thanks to both you and Nicola!

Gary

*** Submitted:

Complete deploy functionality and tests.

This combines three discrete improvements, each with their own commit.

revno: 423
   add machines for units

revno: 424
   add support for passing a YAML string for service configuration, as
would be passed by file parsing

revno: 425
   add a nextChanges function and tests for it in isolation and with
deploy and addUnit.

Note that rev 424 uses js-yaml 2.0.3, which seems to be a lot better
than previous incarnations in my small tests. I decided it was worth
including. As I said in the .min.js file, I committed this to the tree
rather than using npm because the file we needed was not in npm.

rev 425, with the nextChanges, is the most "sketchy." We'll have to see
how that fares when we actually implement the wire protocol, but I think
it should work.

R=teknico, bac
CC=
https://codereview.appspot.com/7741043

https://codereview.appspot.com/7741043/