i'm not going to argue these points any more. i have registered my
objections, and i don't want to block progress. if you still think it's
appropriate, please go ahead.



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 10:58:58, fwereade wrote:
> On 2013/04/16 10:01:00, rog wrote:
> > On 2013/04/16 08:43:40, fwereade wrote:
> > > On 2013/04/15 13:48:21, rog wrote:
> > > > i think this should be just a warning, if anything.
> > > > just because we have private tools present in the current
environment
> > doesn't
> > > > mean we don't want to make tools available to others in the
public bucket.
> > >
> > > Honestly, I question the value of --public regardless. If I were
setting it
> up
> > > I'd have a shared-tools environ that I never actually used other
than to
> sync
> > > tools into, and use its control-bucket as my other environs'
public-bucket.
> > >
> > > Given the current fallback rules, a public sync-tools into an env
with
> private
> > > tools will fail to perform the purpose of sync-tools -- make them
> *available*
> > to
> > > the target env -- and so I think it's actually better to fail
before
> > performing
> > > a ineffectual copy of X MiB via one's local machine.
> >
> > i think if we're asking it to do something, it should darn well do
it.
> > the user may well know best.
> >
> > a warning would be fine, i think.

> A sync-tools into a public bucket being used by a running environ with
tools
> *will not work* from the POV of the user. They ask for the latest
tools, and
> juju does a bunch of work, and they don't get the latest tools.

If they specify --public, they're not asking for the latest tools.
They're asking them to be made available to the "public".

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)

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.

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

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 10:58:58, fwereade wrote:
> On 2013/04/16 10:01:00, rog wrote:
> > On 2013/04/16 08:43:40, fwereade wrote:
> > > On 2013/04/15 13:48:21, rog wrote:
> > > > still pained by this.
> > >
> > > Each non-test file that uses them together also needs to
differentiate
> between
> > > the two situations.
> >
> > the code above demonstrates that statement isn't actually true.

> ...it's the code above *that* that demonstrates that it in fact just
as I stated
> it.

>      case tools.ErrNoTools:
>      case nil, tools.ErrNoMatches:

> > taking a quick look through all the places that use the tools.Err*
values, i
> > don't see any where the errors are particularly useful, and
sometimes they
> make
> > things more obscure.

> 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")

> I haven't yet been convinced that it's sensible to discard the agreed
behaviour,
> but I am becoming convinced that I haven't done it properly and I
should get
> more hardline about copying *available* tools from x such that they
become
> *available* in Y. :)

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".

> > to try and check that i wasn't on crack, i rustled up a branch which
did what
> > i'm suggesting (lose the special errors and the major-version
argument to
> > ListTools). overall (not counting the tests, which i didn't touch)
it's 27
> less
> > lines of code and sync-tools will now work with all tools, not just
the same
> > major version.

> On first inspection that STM to discard bootstrap tools that should be
> considered valid (agent-version should beat dev)

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

> ; it makes sync-tools somewhat
> unhelpful to anyone not using the most recent major version (because
they can't
> copy any useful tools without copying *all* tools)

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

> ; and it makes the first
> major-version upgrade break upgrade-juju in approximately the same way
it breaks
> sync-tools.

only if the above point isn't addressed.

> We will surely reconsider the details when our minds are fully focused
on major
> version upgrades. But today? Let's stick with predictably conservative
> behaviour, please, and not casually duplicate code to do the same
thing in
> multiple places.

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).

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