juju-gui

Code review comment for lp://staging/~gary/juju-gui/sandboxEnv-2

sandboxEnv-2
Merge into trunk

Revision history for this message

Gary Poster (gary) wrote on 2013-03-12:

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

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/

« Back to merge proposal