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?

https://codereview.appspot.com/8748046/