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

Issue 8545043: environs: extract tools package

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

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)

Patch Set 1 #

Patch Set 2 : environs: extract tools package #

Total comments: 25

Patch Set 3 : environs: extract tools package #

Unified diffs Side-by-side diffs Delta from patch set Stats (+721 lines, -635 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/bootstrap.go View 1 2 chunks +2 lines, -1 line 0 comments Download
M cmd/juju/main_test.go View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M cmd/juju/synctools.go View 1 2 5 chunks +9 lines, -9 lines 0 comments Download
M cmd/juju/synctools_test.go View 1 2 5 chunks +10 lines, -24 lines 0 comments Download
M cmd/juju/upgradejuju.go View 3 chunks +5 lines, -6 lines 0 comments Download
M cmd/juju/upgradejuju_test.go View 1 2 7 chunks +33 lines, -42 lines 0 comments Download
M cmd/jujud/agent_test.go View 1 2 chunks +3 lines, -3 lines 0 comments Download
M cmd/jujud/upgrade_test.go View 2 chunks +5 lines, -5 lines 0 comments Download
M environs/dummy/environs.go View 1 2 2 chunks +2 lines, -23 lines 0 comments Download
M environs/ec2/live_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M environs/ec2/local_test.go View 1 3 chunks +4 lines, -2 lines 0 comments Download
M environs/export_test.go View 1 chunk +0 lines, -2 lines 0 comments Download
M environs/jujutest/livetests.go View 1 4 chunks +8 lines, -7 lines 0 comments Download
M environs/maas/environ_test.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M environs/openstack/live_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M environs/openstack/local_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
A environs/storage.go View 1 1 chunk +24 lines, -0 lines 0 comments Download
A environs/storage_test.go View 1 chunk +28 lines, -0 lines 0 comments Download
M environs/testing/tools.go View 1 1 chunk +65 lines, -14 lines 0 comments Download
M environs/tools.go View 5 chunks +4 lines, -269 lines 0 comments Download
A environs/tools/build.go View 1 chunk +147 lines, -0 lines 0 comments Download
A environs/tools/export_test.go View 1 chunk +3 lines, -0 lines 0 comments Download
M environs/tools/list.go View 1 2 2 chunks +0 lines, -3 lines 0 comments Download
A environs/tools/storage.go View 1 2 1 chunk +132 lines, -0 lines 0 comments Download
A environs/tools/storage_test.go View 1 chunk +200 lines, -0 lines 0 comments Download
M environs/tools_test.go View 1 8 chunks +29 lines, -219 lines 0 comments Download

Messages

Total messages: 4
fwereade
Please take a look.
8 years, 7 months ago (2013-04-10 01:31:39 UTC) #1
thumper
Nothing horribly blocking. LGTM, but I'd prefer not to have "fi" as a variable name. ...
8 years, 7 months ago (2013-04-10 06:06:07 UTC) #2
dimitern
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: ...
8 years, 7 months ago (2013-04-11 16:38:42 UTC) #3
fwereade
8 years, 7 months ago (2013-04-12 13:21:14 UTC) #4
*** 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#newcod...
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#newc...
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...
cmd/juju/upgradejuju_test.go:144: // consuming build from source. TODO(fwereade)
better factor environs/tools
On 2013/04/11 16:38:42, dimitern wrote:
> please put the TODO on its own line.

Done.

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
On 2013/04/11 16:38:42, dimitern wrote:
> why do we need this?

Environs without public storage return it.

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#ne...
environs/testing/tools.go:56: // UploadFakeTools puts fake tools into the
supplied storage with a binary
On 2013/04/11 16:38:42, dimitern wrote:
> very nice!

kinda hacky tbh, but very expedient ;p

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.g...
environs/tools/export_test.go:3: var Setenv = setenv
On 2013/04/11 16:38:42, dimitern wrote:
> why not just export it directly?

It's not really a sane addition to the package interface. It's less hassle to
export_test it than it is to copypaste the infrastructure in for internal tests,
especially for something this trivial.

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#ne...
environs/tools/storage.go:22: return toolPrefix + vers.String() + ".tgz"
On 2013/04/11 16:38:42, dimitern wrote:
> maybe const toolExt = ".tgz" above and use it here and in ReadList?

Done.

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go#ne...
environs/tools/storage.go:63: // URLPutter exposes to Upload the relevant
capabilities of an
On 2013/04/10 06:06:07, thumper wrote:
> 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.

I did try adding an environs/storage package, but the sheer volume of changes
that induced kinda put me off. At the moment I'm thinking "environs/interfaces"
(so interfaces.Environ, interfaces.Instance, etc) would be nice.

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go#ne...
environs/tools/storage.go:94: fi, err := f.Stat()
On 2013/04/10 06:06:07, thumper wrote:
> fi is just horrible, and brings back bash syntax.  fileInfo please?

Sure :). Done.

https://codereview.appspot.com/8545043/diff/2001/environs/tools/storage.go#ne...
environs/tools/storage.go:99: log.Infof("environs/tools: built %v (%dkB)",
toolsVersion, (size+512)/1024)
On 2013/04/11 16:38:42, dimitern wrote:
> i never got that magic size+512/1024 - can you explain?

http://play.golang.org/p/lxZDxjgycm
Sign in to reply to this message.

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