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

Issue 7401043: Refactor guts of cmd/juju/get/Run to statecmd.

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 9 months ago by bac
Modified:
6 years, 9 months ago
Reviewers:
mp+149695, dfc, jameinel, fwereade, dimitern
Visibility:
Public.

Description

Refactor guts of cmd/juju/get/Run to statecmd. This is the first of two branches to provide API support for 'juju get'. This one does the preliminary work of moving the common code to statecmd. The next branch will add support in client and apiserver. Did some drive-by changes including typo fixes that were visible in output and added some content to the README. https://code.launchpad.net/~bac/juju-core/add-get/+merge/149695 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 31

Patch Set 2 : Refactor guts of cmd/juju/get/Run to statecmd. #

Total comments: 4

Patch Set 3 : Refactor guts of cmd/juju/get/Run to statecmd. #

Patch Set 4 : Refactor guts of cmd/juju/get/Run to statecmd. #

Patch Set 5 : Refactor guts of cmd/juju/get/Run to statecmd. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+183 lines, -62 lines) Patch
M CONTRIBUTING View 1 6 chunks +8 lines, -12 lines 0 comments Download
M README View 1 2 chunks +17 lines, -0 lines 0 comments Download
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/get.go View 1 2 chunks +17 lines, -48 lines 0 comments Download
M state/api/client.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M state/statecmd/config_test.go View 1 2 chunks +64 lines, -1 line 0 comments Download
A state/statecmd/get.go View 1 2 1 chunk +74 lines, -0 lines 0 comments Download

Messages

Total messages: 11
bac
Please take a look.
6 years, 9 months ago (2013-02-20 21:16:14 UTC) #1
dfc
Thank you. One comment about the api for ServiceGet but otherwise looks good. https://codereview.appspot.com/7401043/diff/1/cmd/juju/get.go File ...
6 years, 9 months ago (2013-02-21 01:51:31 UTC) #2
jameinel
LGTM, I think I agree with dfc that returning the struct rather than taking it ...
6 years, 9 months ago (2013-02-21 12:39:36 UTC) #3
dimitern
Overall LGTM, modulo some trivials. https://codereview.appspot.com/7401043/diff/1/CONTRIBUTING File CONTRIBUTING (left): https://codereview.appspot.com/7401043/diff/1/CONTRIBUTING#oldcode62 CONTRIBUTING:62: Alternatively, you can combine ...
6 years, 9 months ago (2013-02-21 13:26:16 UTC) #4
fwereade
Looking good, grab me online if anything's unclear. I'm still trying to make sense of ...
6 years, 9 months ago (2013-02-21 13:41:37 UTC) #5
bac
Thanks for the reviews. I have addressed all of the issues. The text in the ...
6 years, 9 months ago (2013-02-21 15:32:54 UTC) #6
bac
Please take a look.
6 years, 9 months ago (2013-02-21 17:23:04 UTC) #7
fwereade
LGTM, just a couple of typos https://codereview.appspot.com/7401043/diff/12001/state/statecmd/get.go File state/statecmd/get.go (right): https://codereview.appspot.com/7401043/diff/12001/state/statecmd/get.go#newcode5 state/statecmd/get.go:5: // when the ...
6 years, 9 months ago (2013-02-21 21:07:17 UTC) #8
bac
Fixed. Thanks. https://codereview.appspot.com/7401043/diff/12001/state/statecmd/get.go File state/statecmd/get.go (right): https://codereview.appspot.com/7401043/diff/12001/state/statecmd/get.go#newcode5 state/statecmd/get.go:5: // when the command-line commands can invoking ...
6 years, 9 months ago (2013-02-21 21:15:42 UTC) #9
dfc
LGTM. Thank you
6 years, 9 months ago (2013-02-21 23:27:10 UTC) #10
bac
6 years, 9 months ago (2013-02-22 12:40:51 UTC) #11
*** Submitted:

Refactor guts of cmd/juju/get/Run to statecmd.

This is the first of two branches to provide API support for 'juju get'.  This
one does the preliminary work of moving the common code to statecmd.  The next
branch will add support in client and apiserver.

Did some drive-by changes including typo fixes that were visible in output and
added some content to the README.

R=dfc, jameinel, dimitern, fwereade
CC=
https://codereview.appspot.com/7401043
Sign in to reply to this message.

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