Code review comment for lp://staging/~fwereade/juju-core/environs-tools-final-replacement
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
9199653
demo site
(Get the code!)
It continues... is there some use case I have failed to properly take
into account?
https:/
File cmd/juju/
https:/
cmd/juju/
public tools would be ignored")
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 want fresh tools to run on a real environ, they can
destroy-
using the environment purely as a frontend to a public tools bucket,
they'll never have private tools messing everything up.
https:/
cmd/juju/
tools.ErrNoTools:
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 feel twitchy enough about the magic
source env and hardcoded use of its public-storage, but I managed to
resist the temptation to add a --source env-name feature (which would
default to the magic env, and in all cases would cause sync-tools to
draw from only the *available* tools in the source env).
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. :)
> 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); 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);
and it makes the first major-version upgrade break upgrade-juju in
approximately the same way it breaks sync-tools.
I think that fixing all these as well would perhaps make the change a
little less compelling, not to mention making the effects less certainly
predictable.
> in particular, putting the major-version matching inside Match seems
like the
> Right Thing to do.
> lp:~rogpeppe/juju-core/fwereade-less-errors
+0.5 in theory. In practice, looking for major-version-
one specific bucket is the behaviour we need right now, and I'm not sure
it's a win to take that behaviour -- currently implemented OAOO -- and
smear it out across all the places that'll have to use it... just to get
rid of an error you don't like, that precisely differentiates the cases
we care about both in environs/tools.go and in cmd/juju/
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.
https:/