Code review comment for lp://staging/~fwereade/juju-core/environs-tools-storage

Revision history for this message
William Reade (fwereade) wrote :

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

« Back to merge proposal