Merge lp://staging/~fwereade/juju-core/environs-tools-storage into lp://staging/~juju/juju-core/trunk

Reviewers: mp+157770_code.launchpad.net,

Message:
Please take a look.

Description:
environs: extract tools package

This is much simpler than it looks. Motivation is as follows:

  1) I'm trying to make our tools logic consistent, so we have some sort
of
   chance of different parts of the system acting in concert (bootstrap,
 start-instance, sync-tools, upgrade-juju, juju upgrades, ...?). It is
 currently very difficult to build a mental model of how these things
are
 related, and a common vocabulary will help.

  2) I'd be crazy to try to do everything at once (ha!). So I'll leave
the
   existing tools *selection* logic (FindTools, BestTools) in place while
 I focus on just uploads/downloads.

  3) Move tool-storagey code from environs to environs/tools:

   * environs.listTools -> tools.ReadList
     (environs.ListTools now uses tools.ReadList)
   * environs.PutTools -> tools.Upload
     (lots of code moved to storage.go and build.go in tools/)
   * environs.ToolsStoragePath -> tools.StorageName
     (it's a name, not a path, according to the storage metaphor)

   ...hmm. Lots of renames.

  4) But wait, I can't do this, I need to use environs.Storage, and
there's
   an import loop. To keep the diff small, define tools.URLLister and
 tools.URLPutter.

  6) The tests are starting to look somewhat repetitive, and I fear
subtle
   differences creeping in. Let's bulk up environs/testing a little.
   Ah, that's better.

 From a review perspective, this branch is largely mechanical. The only
code
that is new, as opposed to moved or mechanically replaced, is:

   * the tests for tools.ReadList (made explicit now it's exported).
   * the tests for environs.EmptyStorage (yes, it's trivial. yes, it
should
     still be tested.)
   * environs/testing/tools.go (mostly new, and used in several places).
   * UpgradeJujuCommand (surprising behaviour corrected, clarifying test
    added).

https://code.launchpad.net/~fwereade/juju-core/environs-tools-storage/+merge/157770

Requires:
https://code.launchpad.net/~fwereade/juju-core/environs-tools-list/+merge/157964

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8545043/

Affected files:
   A [revision details]
   M cmd/juju/bootstrap.go
   M cmd/juju/main_test.go
   M cmd/juju/sync_tools.go
   M cmd/juju/sync_tools_test.go
   M cmd/juju/upgradejuju.go
   M cmd/juju/upgradejuju_test.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/upgrade_test.go
   M environs/dummy/environs.go
   M environs/ec2/live_test.go
   M environs/ec2/local_test.go
   M environs/export_test.go
   M environs/jujutest/livetests.go
   M environs/openstack/live_test.go
   M environs/openstack/local_test.go
   A environs/storage.go
   A environs/storage_test.go
   M environs/testing/tools.go
   M environs/tools.go
   A environs/tools/build.go
   A environs/tools/export_test.go
   M environs/tools/list.go
   A environs/tools/storage.go
   A environs/tools/storage_test.go
   M environs/tools_test.go

Nothing horribly blocking. LGTM, but I'd prefer not to have "fi" as a
variable name.

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go#newcode187
cmd/juju/main_test.go:187: fullmsg := fmt.Sprintf(`(.*\n)*.*ERROR
JUJU:juju:bootstrap juju bootstrap command failed: %s\n`, msg)
I've found myself using (.|\n)*, one less *. YMMV

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go#newcode63
environs/tools/storage.go:63: // URLPutter exposes to Upload the
relevant capabilities of an
I'm beginning to get an idea about how we can handle interfaces better,
relating to the interface segregation principle, and the dependency
inversion principle. Oakland time.

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go#newcode94
environs/tools/storage.go:94: fi, err := f.Stat()
fi is just horrible, and brings back bash syntax. fileInfo please?

https://codereview.appspot.com/8545043/

Nice! LGTM with a number of mostly trivial suggestions.

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go#newcode187
cmd/juju/main_test.go:187: fullmsg := fmt.Sprintf(`(.*\n)*.*ERROR
JUJU:juju:bootstrap juju bootstrap command failed: %s\n`, msg)
On 2013/04/10 06:06:07, thumper wrote:
> I've found myself using (.|\n)*, one less *. YMMV

+10

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go#newcode200
cmd/juju/main_test.go:200: fullmsg := fmt.Sprintf(`(.*\n)*.*ERROR
JUJU:juju:bootstrap juju bootstrap command failed: %s\n`, msg)
ditto

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/sync_tools_test.go
File cmd/juju/sync_tools_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/sync_tools_test.go#newcode181
cmd/juju/sync_tools_test.go:181: // newest tools added to the public
bucket
good catch!

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju.go#newcode88
cmd/juju/upgradejuju.go:88: } else if c.Version == (version.Number{}) {
var emptyVersion = version.Number{} and use it here; helps readability?

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju_test.go#newcode120
cmd/juju/upgradejuju_test.go:120: expectVersion: "2.0.1",
nice!

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju_test.go#newcode144
cmd/juju/upgradejuju_test.go:144: // consuming build from source.
TODO(fwereade) better factor environs/tools
please put the TODO on its own line.

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju_test.go#newcode221
cmd/juju/upgradejuju_test.go:221: envtesting.RemoveTools(c,
s.Conn.Environ.PublicStorage().(environs.Storage))
this cast is kinda crap, but i know public storage is supposed to be
read only.

https://codereview.appspot.com/8545043/diff/2001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/8545043/diff/2001/environs/storage.go#newcode1
environs/storage.go:1: package environs
why do we need this?

https://codereview.appspot.com/8545043/diff/2001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/8545043/diff/2001/environs/testing/tools.go#newcode56
environs/testing/tools.go:56: // UploadFakeTools puts fake tools into
the supplied storage with a binary
very nice!

https://codereview.appspot.com/8545043/diff/2001/environs/tools/export_test.go
File environs/tools/export_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/environs/tools/export_test.go#newcode3
environs/tools/export_test.go:3: var Setenv = setenv
why not just export it directly?

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go#newcode22
environs/tools...

*** Submitted:

environs: extract tools package

This is much simpler than it looks. Motivation is as follows:

  1) I'm trying to make our tools logic consistent, so we have some sort
of
   chance of different parts of the system acting in concert (bootstrap,
 start-instance, sync-tools, upgrade-juju, juju upgrades, ...?). It is
 currently very difficult to build a mental model of how these things
are
 related, and a common vocabulary will help.

  2) I'd be crazy to try to do everything at once (ha!). So I'll leave
the
   existing tools *selection* logic (FindTools, BestTools) in place while
 I focus on just uploads/downloads.

  3) Move tool-storagey code from environs to environs/tools:

   * environs.listTools -> tools.ReadList
     (environs.ListTools now uses tools.ReadList)
   * environs.PutTools -> tools.Upload
     (lots of code moved to storage.go and build.go in tools/)
   * environs.ToolsStoragePath -> tools.StorageName
     (it's a name, not a path, according to the storage metaphor)

   ...hmm. Lots of renames.

  4) But wait, I can't do this, I need to use environs.Storage, and
there's
   an import loop. To keep the diff small, define tools.URLLister and
 tools.URLPutter.

  6) The tests are starting to look somewhat repetitive, and I fear
subtle
   differences creeping in. Let's bulk up environs/testing a little.
   Ah, that's better.

 From a review perspective, this branch is largely mechanical. The only
code
that is new, as opposed to moved or mechanically replaced, is:

   * the tests for tools.ReadList (made explicit now it's exported).
   * the tests for environs.EmptyStorage (yes, it's trivial. yes, it
should
     still be tested.)
   * environs/testing/tools.go (mostly new, and used in several places).
   * UpgradeJujuCommand (surprising behaviour corrected, clarifying test
    added).

R=thumper, dimitern
CC=
https://codereview.appspot.com/8545043

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go
File cmd/juju/main_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/main_test.go#newcode187
cmd/juju/main_test.go:187: fullmsg := fmt.Sprintf(`(.*\n)*.*ERROR
JUJU:juju:bootstrap juju bootstrap command failed: %s\n`, msg)
On 2013/04/10 06:06:07, thumper wrote:
> I've found myself using (.|\n)*, one less *. YMMV

Done.

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju.go#newcode88
cmd/juju/upgradejuju.go:88: } else if c.Version == (version.Number{}) {
On 2013/04/11 16:38:42, dimitern wrote:
> var emptyVersion = version.Number{} and use it here; helps
readability?

If I were going to do that I'd probably favour a const version.Empty...
I shall see how big/ugly a change it is and decide then :).

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/8545043/diff/2001/cmd/juju/upgradejuju_test.go#newcode144
cmd/juju/upgradejuju_test.go:144: // consuming build from source.
TODO(fwereade) better factor environs/tools
On 2013/04/11 16:38:42...