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

Issue 11561044: Make agent/tools and remove state.Tools

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by thumper
Modified:
6 years, 2 months ago
Reviewers:
mp+176111, jtv.canonical, wallyworld, rog
Visibility:
Public.

Description

Make agent/tools and remove state.Tools This branch makes an agent/tools package that is all things tools. The environs/tools package is merged in there. There is a test to ensure that we don't bring in extra package dependencies to tools, right now just "utils/set" and "version". https://code.launchpad.net/~thumper/juju-core/agent-tools/+merge/176111 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+489 lines, -416 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M agent/agent.go View 3 chunks +3 lines, -2 lines 0 comments Download
M agent/tools/build.go View 1 chunk +0 lines, -4 lines 0 comments Download
M agent/tools/build_test.go View 1 chunk +1 line, -1 line 0 comments Download
M agent/tools/diskmanager.go View 2 chunks +3 lines, -5 lines 1 comment Download
M agent/tools/diskmanager_test.go View 5 chunks +10 lines, -10 lines 0 comments Download
M agent/tools/export_test.go View 0 chunks +-1 lines, --1 lines 0 comments Download
M agent/tools/list.go View 4 chunks +8 lines, -9 lines 0 comments Download
M agent/tools/list_test.go View 6 chunks +22 lines, -27 lines 1 comment Download
A agent/tools/marshal.go View 1 chunk +40 lines, -0 lines 0 comments Download
A agent/tools/marshal_test.go View 1 chunk +57 lines, -0 lines 0 comments Download
A agent/tools/package_test.go View 1 chunk +14 lines, -0 lines 0 comments Download
M agent/tools/storage.go View 5 chunks +4 lines, -5 lines 2 comments Download
M agent/tools/storage_test.go View 3 chunks +11 lines, -14 lines 0 comments Download
M agent/tools/tools.go View 1 chunk +5 lines, -1 line 0 comments Download
M agent/tools/tools_test.go View 6 chunks +67 lines, -62 lines 1 comment Download
M agent/tools/toolsdir.go View 7 chunks +8 lines, -9 lines 0 comments Download
D agent/tools_compat_test.go View 1 chunk +0 lines, -35 lines 0 comments Download
M cmd/juju/bootstrap_test.go View 1 chunk +1 line, -1 line 0 comments Download
M cmd/juju/status.go View 3 chunks +5 lines, -2 lines 0 comments Download
M cmd/juju/status_test.go View 3 chunks +3 lines, -2 lines 0 comments Download
M cmd/juju/synctools.go View 4 chunks +5 lines, -6 lines 0 comments Download
M cmd/juju/synctools_test.go View 1 chunk +1 line, -1 line 0 comments Download
M cmd/juju/upgradejuju.go View 1 chunk +1 line, -1 line 0 comments Download
M cmd/juju/upgradejuju_test.go View 2 chunks +5 lines, -4 lines 0 comments Download
M cmd/jujud/agent.go View 2 chunks +2 lines, -1 line 0 comments Download
M cmd/jujud/agent_test.go View 7 chunks +16 lines, -16 lines 0 comments Download
M cmd/jujud/machine_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M cmd/jujud/unit_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M cmd/jujud/upgrade.go View 8 chunks +14 lines, -14 lines 0 comments Download
M cmd/jujud/upgrade_test.go View 7 chunks +13 lines, -14 lines 0 comments Download
M container/lxc/lxc.go View 6 chunks +7 lines, -5 lines 0 comments Download
M container/lxc/lxc_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M environs/azure/customdata_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M environs/azure/environ.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/cloudinit/cloudinit.go View 4 chunks +4 lines, -3 lines 0 comments Download
M environs/cloudinit/cloudinit_test.go View 4 chunks +8 lines, -5 lines 0 comments Download
M environs/cloudinit_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M environs/ec2/ec2.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/jujutest/livetests.go View 5 chunks +5 lines, -5 lines 0 comments Download
M environs/local/environ.go View 3 chunks +7 lines, -6 lines 0 comments Download
M environs/maas/environ.go View 2 chunks +2 lines, -2 lines 0 comments Download
M environs/maas/environ_test.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/openstack/export_test.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/openstack/provider.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/testing/storage.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/testing/tools.go View 2 chunks +8 lines, -7 lines 0 comments Download
M environs/tools.go View 2 chunks +2 lines, -3 lines 0 comments Download
M environs/tools_test.go View 2 chunks +3 lines, -4 lines 0 comments Download
M state/api/params/internal.go View 1 chunk +1 line, -1 line 0 comments Download
M state/api/upgrader/upgrader_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M state/apiserver/upgrader/upgrader.go View 2 chunks +13 lines, -12 lines 0 comments Download
M state/apiserver/upgrader/upgrader_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M state/machine.go View 4 chunks +5 lines, -4 lines 0 comments Download
M state/machine_test.go View 3 chunks +3 lines, -2 lines 0 comments Download
M state/state.go View 1 chunk +0 lines, -33 lines 0 comments Download
M state/state_test.go View 2 chunks +2 lines, -1 line 0 comments Download
M state/tools_test.go View 5 chunks +6 lines, -39 lines 0 comments Download
M state/unit.go View 4 chunks +4 lines, -3 lines 0 comments Download
A testing/imports.go View 1 chunk +44 lines, -0 lines 3 comments Download
M worker/deployer/simple.go View 4 chunks +5 lines, -4 lines 0 comments Download
M worker/deployer/simple_test.go View 3 chunks +4 lines, -3 lines 0 comments Download
M worker/provisioner/lxc-broker.go View 3 chunks +3 lines, -2 lines 0 comments Download
M worker/provisioner/lxc-broker_test.go View 3 chunks +3 lines, -3 lines 0 comments Download
M worker/provisioner/provisioner.go View 2 chunks +3 lines, -3 lines 0 comments Download
M worker/uniter/tools_test.go View 2 chunks +3 lines, -3 lines 0 comments Download
M worker/uniter/uniter.go View 2 chunks +2 lines, -2 lines 0 comments Download
M worker/uniter/uniter_test.go View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 5
thumper
Please take a look.
6 years, 3 months ago (2013-07-22 04:32:36 UTC) #1
wallyworld
LGTM https://codereview.appspot.com/11561044/diff/1/agent/tools/storage.go File agent/tools/storage.go (right): https://codereview.appspot.com/11561044/diff/1/agent/tools/storage.go#newcode84 agent/tools/storage.go:84: // environs.Storage; it exists to avoid a dependency ...
6 years, 3 months ago (2013-07-22 04:52:03 UTC) #2
thumper
https://codereview.appspot.com/11561044/diff/1/agent/tools/storage.go File agent/tools/storage.go (right): https://codereview.appspot.com/11561044/diff/1/agent/tools/storage.go#newcode84 agent/tools/storage.go:84: // environs.Storage; it exists to avoid a dependency on ...
6 years, 3 months ago (2013-07-22 04:55:38 UTC) #3
jtv.canonical
LGTM — of course I can't comment on the actual change, because my brain stopped ...
6 years, 3 months ago (2013-07-23 05:41:55 UTC) #4
rog
6 years, 2 months ago (2013-07-23 16:34:45 UTC) #5
LGTM with some suggestions below.

I think this is a good example of how we can
refactor without fear given a statically typed
language.

https://codereview.appspot.com/11561044/diff/1/agent/tools/diskmanager.go
File agent/tools/diskmanager.go (right):

https://codereview.appspot.com/11561044/diff/1/agent/tools/diskmanager.go#new...
agent/tools/diskmanager.go:29: return UnpackTools(d.dataDir, tools, r)
nice change

https://codereview.appspot.com/11561044/diff/1/agent/tools/list_test.go
File agent/tools/list_test.go (right):

https://codereview.appspot.com/11561044/diff/1/agent/tools/list_test.go#newcode7
agent/tools/list_test.go:7: gc "launchpad.net/gocheck"
in this already huge CL, it would perhaps be nice to keep unnecessary changes to
a minimum, though I appreciate these are fairly automatic.

https://codereview.appspot.com/11561044/diff/1/agent/tools/tools_test.go
File agent/tools/tools_test.go (right):

https://codereview.appspot.com/11561044/diff/1/agent/tools/tools_test.go#newc...
agent/tools/tools_test.go:13: gc "launchpad.net/gocheck"
again, it would be nice to restrict changes to those directly relevant to this
CL.

https://codereview.appspot.com/11561044/diff/1/testing/imports.go
File testing/imports.go (right):

https://codereview.appspot.com/11561044/diff/1/testing/imports.go#newcode16
testing/imports.go:16: // imported by the packageName parameter.  The resulting
list removes the
// imported by the package with the given name.
?

https://codereview.appspot.com/11561044/diff/1/testing/imports.go#newcode19
testing/imports.go:19: var imports []string
i don't think you need to traverse SrcDirs manually.

i think you probably want something like the following:

pkg, err := build.Import(packageName)
if err != nil {
   c.Fatalf("cannot import %q: %v", packageName, err)
}
[...]
for _, name := range pkg.Imports {
[...]

https://codereview.appspot.com/11561044/diff/1/testing/imports.go#newcode31
testing/imports.go:31: c.Logf(packageName + " not found")
probably c.Fatalf, but no need if you make the change above.
Sign in to reply to this message.

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