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

Issue 8604043: environs/tools: new package

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by fwereade
Modified:
9 years, 7 months ago
Reviewers:
mp+157964, dfc, thumper
Visibility:
Public.

Description

environs/tools: new package Tools selection logic implemented for `juju sync-tools` moved into its own package, and was expanded somewhat. https://code.launchpad.net/~fwereade/juju-core/environs-tools-list/+merge/157964 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : environs/tools: new package #

Total comments: 43

Patch Set 3 : environs/tools: new package #

Patch Set 4 : environs/tools: new package #

Unified diffs Side-by-side diffs Delta from patch set Stats (+374 lines, -122 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/synctools.go View 1 2 3 chunks +2 lines, -36 lines 0 comments Download
M cmd/juju/synctools_test.go View 1 2 2 chunks +0 lines, -82 lines 0 comments Download
M environs/tools.go View 3 chunks +5 lines, -4 lines 0 comments Download
A environs/tools/list.go View 1 2 1 chunk +136 lines, -0 lines 0 comments Download
A environs/tools/list_test.go View 1 2 1 chunk +229 lines, -0 lines 0 comments Download

Messages

Total messages: 9
fwereade
Please take a look.
9 years, 7 months ago (2013-04-10 01:04:36 UTC) #1
dfc
LGTM. Nice cleanup.
9 years, 7 months ago (2013-04-10 01:36:42 UTC) #2
thumper
Several comment tweaks, and one potential problem with finding newest tools. Obviously you are missing ...
9 years, 7 months ago (2013-04-10 04:32:51 UTC) #3
rog
very nice. lots of remarks here but only suggestions for naming and comments. what do ...
9 years, 7 months ago (2013-04-10 08:30:31 UTC) #4
TheMue
Feels good. Some additional comments. https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go File environs/tools/list.go (right): https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newcode14 environs/tools/list.go:14: func (src List) String() ...
9 years, 7 months ago (2013-04-10 09:25:00 UTC) #5
dimitern
Looking good, although I agree with most other comments I have one of my own ...
9 years, 7 months ago (2013-04-10 10:03:02 UTC) #6
fwereade
Please take a look. https://codereview.appspot.com/8604043/diff/2001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8604043/diff/2001/cmd/juju/sync_tools.go#newcode1 cmd/juju/sync_tools.go:1: package main On 2013/04/10 10:03:02, ...
9 years, 7 months ago (2013-04-10 23:07:29 UTC) #7
thumper
> Public opinion is clearly against me here, so I'll change it, but offer the ...
9 years, 7 months ago (2013-04-10 23:39:32 UTC) #8
fwereade
9 years, 7 months ago (2013-04-11 00:04:28 UTC) #9
*** Submitted:

environs/tools: new package

Tools selection logic implemented for `juju sync-tools` moved into its own
package, and was expanded somewhat.

R=dfc, thumper, rog, TheMue, dimitern
CC=
https://codereview.appspot.com/8604043
Sign in to reply to this message.

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