Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(504)

Issue 8684043: Add destroy_service to fakebackend.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 7 months ago by bac
Modified:
8 years, 7 months ago
Reviewers:
bcsaller, mp+158488, matthew.scott
Visibility:
Public.

Description

Add destroy_service to fakebackend. https://code.launchpad.net/~bac/juju-gui/fake-destroy-service/+merge/158488 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 6

Patch Set 2 : Add destroy_service to fakebackend. #

Total comments: 5

Patch Set 3 : Add destroy_service to fakebackend. #

Total comments: 2

Patch Set 4 : Add destroy_service to fakebackend. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+148 lines, -5 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M app/store/env/fakebackend.js View 1 2 3 2 chunks +38 lines, -3 lines 0 comments Download
M app/store/env/sandbox.js View 2 chunks +11 lines, -1 line 0 comments Download
M test/test_fakebackend.js View 1 2 3 2 chunks +64 lines, -0 lines 0 comments Download
M test/test_sandbox.js View 1 2 2 chunks +33 lines, -1 line 0 comments Download

Messages

Total messages: 8
bac
Please take a look.
8 years, 7 months ago (2013-04-11 20:57:38 UTC) #1
bcsaller
Thanks for this, it still needs work, its missing one key element. The other is ...
8 years, 7 months ago (2013-04-11 21:20:11 UTC) #2
bac
Please take a look. https://codereview.appspot.com/8684043/diff/1/app/store/env/fakebackend.js File app/store/env/fakebackend.js (right): https://codereview.appspot.com/8684043/diff/1/app/store/env/fakebackend.js#newcode392 app/store/env/fakebackend.js:392: return {error: 'Invalid service id.'}; ...
8 years, 7 months ago (2013-04-12 17:24:24 UTC) #3
bcsaller
LGTM but I'm surprised the sandbox tests pass w/o the name change. If you just ...
8 years, 7 months ago (2013-04-12 17:45:52 UTC) #4
bac
Please take a look. https://codereview.appspot.com/8684043/diff/6001/test/test_sandbox.js File test/test_sandbox.js (right): https://codereview.appspot.com/8684043/diff/6001/test/test_sandbox.js#newcode791 test/test_sandbox.js:791: assert.equal(parsed.service_name, 'wordpress'); On 2013/04/12 17:45:52, ...
8 years, 7 months ago (2013-04-12 18:50:19 UTC) #5
matthew.scott
LGTM, though I have a question. It looks like this will leave units and machines ...
8 years, 7 months ago (2013-04-12 19:11:25 UTC) #6
bac
On 2013/04/12 19:11:25, matthew.scott wrote: > LGTM, though I have a question. It looks like ...
8 years, 7 months ago (2013-04-15 13:27:05 UTC) #7
bac
8 years, 7 months ago (2013-04-15 13:34:09 UTC) #8
*** Submitted:

Add destroy_service to fakebackend.

R=bcsaller, matthew.scott
CC=
https://codereview.appspot.com/8684043

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

https://codereview.appspot.com/8684043/diff/12001/app/store/env/fakebackend.j...
app/store/env/fakebackend.js:382: @method destroyService
On 2013/04/12 19:11:25, matthew.scott wrote:
> Trivial: I think one comment style or the other would be nice, but both is a
> little weird :)

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b