Code review comment for lp://staging/~fwereade/juju-core/environs-tools-final-replacement

On 17 April 2013 10:26, William Reade <email address hidden> wrote:
> I probably shouldn't keep banging on about this, but some of it's
> actually really important.

Agreed. I hope this discussion is actually helping us come to
a shared understanding of these issues.

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

The public bucket is shared between many environments
(that's kind of the whole point of it).

Just because my current environment is using tools from
its private bucket doesn't mean that I don't want to make
tools available in the public bucket where other environments
that use it can then have access to them. Those other
environments might potentially want to bootstrap with a different major version
too.

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

That is true. But that perspective is not that only valid one, I believe.

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

As above, you are assuming that the only reason for uploading
to the public bucket is so that *this* environment can use them.

>> 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'm not sure I understand how what you're saying there relates to
what I suggested.

> I hope I
> do not need to reiterate that the difference between "working" and "not
> working", as described above, is in fact user-observable.

The issues above (returning an error when uploading to the public
bucket if there's stuff in the private bucket) are orthogonal to
the List suggestions, aren't they?

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

I guess the question is "are tools with a different major version
'available' to the target environment?"

I'd suggest that they are, even if we can't run them.
When we allow major-version upgrades, we *will* be able
to run them, in fact, but we will only be able to upload them
by changing the local CLI client, at which point all those
other tools suddenly become invisible.

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

I wasn't refactoring for SLOC - the improvement in that metric was just a nice
(and I think indicative) result. I was suggesting that things could
be simpler, and I believe the result *is* simpler.

I'm still not sure which duplicated code you're talking about BTW.

I'm not going to go into this any more though - it's
was just a suggestion that I thought you might see and think
was evidently nicer. Given that's not the case, let's forget it.

> 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?

I favour code with simple, understandable and general functionality.
I try to avoid exporting functions which are tailored very specifically for
the characteristics of one particular problem, preferring to provide
tools that work in the most natural way possible in their own terms.

Sometimes this might result in a little more logic outside
the function, but I think it is usually more than paid for in the resulting
simplicity and understandability of the whole system.

Of course, it doesn't always work out that way. We're always
exploring new paths. My own explorations have undoubtedly
led us into some bristly thorn bushes at times, and for that I am sorry.