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

Issue 8748046: cmd/juju: sync-tools fixes

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by fwereade
Modified:
10 years, 11 months ago
Reviewers:
dimitern, mp+158830, rog
Visibility:
Public.

Description

cmd/juju: sync-tools fixes sync-tools now requires the --dev flag before copying development versions, and no longer copies into a public bucket when the contents of that bucket are not available to the target environment. This branch removes the last of the fuzzy multi-bucket tools code. https://code.launchpad.net/~fwereade/juju-core/environs-tools-final-replacement/+merge/158830 Requires: https://code.launchpad.net/~fwereade/juju-core/environs-tools-provisioning-integration/+merge/158805 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 21

Patch Set 2 : cmd/juju: sync-tools fixes #

Patch Set 3 : cmd/juju: sync-tools fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+191 lines, -671 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/synctools.go View 3 chunks +40 lines, -20 lines 0 comments Download
M cmd/juju/synctools_test.go View 1 2 chunks +131 lines, -142 lines 0 comments Download
M environs/tools.go View 1 2 5 chunks +18 lines, -107 lines 0 comments Download
M environs/tools_test.go View 1 2 2 chunks +0 lines, -402 lines 0 comments Download

Messages

Total messages: 9
fwereade
Please take a look.
10 years, 11 months ago (2013-04-14 23:57:49 UTC) #1
rog
LGTM with the "private tools present" error fixed and a couple more minor suggestions. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go ...
10 years, 11 months ago (2013-04-15 13:48:21 UTC) #2
dimitern
LGTM with a few trivials. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go File cmd/juju/synctools_test.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newcode116 cmd/juju/synctools_test.go:116: "1.0.0-precise-amd64", "1.0.0-quantal-amd64", "1.0.0-quantal-i386") please, ...
10 years, 11 months ago (2013-04-15 15:00:54 UTC) #3
fwereade
Please take a look. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public ...
10 years, 11 months ago (2013-04-16 08:43:40 UTC) #4
rog
https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") ...
10 years, 11 months ago (2013-04-16 10:01:00 UTC) #5
fwereade
It continues... is there some use case I have failed to properly take into account? ...
10 years, 11 months ago (2013-04-16 10:58:58 UTC) #6
rog
i'm not going to argue these points any more. i have registered my objections, and ...
10 years, 11 months ago (2013-04-16 11:54:24 UTC) #7
fwereade
*** Submitted: cmd/juju: sync-tools fixes sync-tools now requires the --dev flag before copying development versions, ...
10 years, 11 months ago (2013-04-17 07:03:14 UTC) #8
fwereade
10 years, 11 months ago (2013-04-17 09:22:43 UTC) #9
I probably shouldn't keep banging on about this, but some of it's actually
really important.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150
cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public
tools would be ignored")
On 2013/04/16 11:54:24, rog wrote:
> If they specify --public, they're not asking for the latest tools.
> They're asking them to be made available to the "public".

sync-tools is an environment-level command, which is intended to make tools
available to that environment. The --public flag causes it to use the
public-bucket in doing so.

> I can easily think of a situation where I'd want to use synctools
> with an existing environment bootstrapped with --upload-tools;
> in fact, that's quite possibly the way I'd *normally* use them
> (juju bootstrap --upload-tools; check it works; juju sync-tools --public)

Please explain what purpose this serves.

> The worst possible down side of letting the sync carry on is that it spends
> a while copying tools (and you'd get a warning). I can't see that as a
> significant problem.

The problem is that, from the perspective of the user who wants to use newer
tools, the command has *failed* despite reporting success.

> Aborting with an error seems to me to be patronising behaviour.

The user has set up their environment such that it cannot use public tools.
Wasting the user's time and bandwidth to do something that cannot possibly work
and then throwing up our hands and saying "but we warned you" doesn't seem
ideal. TBH, I think that has a pretty clear "I told you so but you just wouldn't
listen" subtext that's pretty patronising itself.

https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161
cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools:
On 2013/04/16 11:54:24, rog wrote:
> On 2013/04/16 10:58:58, fwereade wrote:
> > AIUI, what we agreed in atlanta was that private tools would render public
> ones
> > inaccessible. I don't think that adding an exception to that rule makes
> anything
> > clearer...
> 
> i don't think i'm making an exception to that rule.
> if anything i think it's making *less* exceptions.
> the user-observable behaviour with my suggested changes
> should be exactly the same as your branch is now
> (with the exception that --all actually means "all")

You are introducing a fuzzy and unnamed concept that maps to a group of
environments that happen to share some arbitrary convenient config setting, and
you are conflating those with actual environments. I hope I do not need to
reiterate that the difference between "working" and "not working", as described
above, is in fact user-observable.

> that's leaning hard on your particular definition of "available", meaning
> "available to this particular client". i'd prefer a more general definition:
> "available to some client".

I mean "available to the target environment". If we follow your definition, we
don't need sync-tools at all, because any syncable tool is inherently accessible
by *some* environment. But I don't see how that helps users.

> yes, i missed a "return list, nil" in that if statement.

> true, but trivially addressed by adding to the filter above;
> it adds 4 lines of code.

> only if the above point isn't addressed.

> personally i prefer to have less code overall, even if there's some
duplication
> in it (not that i'm sure what "casually duplicated" code you're referring to
> here).

Actually this is a textbook example. In refactoring for SLOC you have introduced
3 bugs, 2 of which are basically the same but it's now impossible to tell that
they are the same. Say your proposal were to land; at some stage we'd get a
report of one of the 2 identical ones, and someone would fix it. But they
probably wouldn't spot the duplicated bug elsewhere, so when that one's reported
someone else is likely to pick it up and fix that in isolation.

But those two fixes will probably not be identical, and might not be identical
in effect -- and even if they are the situation is no better, because a
behaviour change to one will not necessarily be understood to be relevant to the
other. And so the tools-selection logic will get a reputation in our minds as
flaky and inconsistent, and it'll be a constant (if subtle) drag on
development...

...and eventually some poor sod will have to make some sense of it all, and find
all the places that get tools.Lists and how they're used and manipulated, and
figure out which inconsistent bits are bugs, and which are just implemented
differently; and turn that whole mess into something that can be understood in
something resembling isolation. (please note: this is what I have *just been
doing*, for *this exact functionality*)

I do not actually *like* doing that sort of work, because it is hard and ugly
and only pays off over the long term, but I do it because I am all too familiar
with the costs of failing to do so.

All of the above is why duplication is so upsetting to many people -- because
they know from bitter experience where it leads. I'm still not sure why it is
that you tend to favour this sort of code; have you never experienced the sorts
of scenarios I'm describing?
Sign in to reply to this message.

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