cmd/juju: make upgrade-juju compatible with 1.16

cmd/juju: make upgrade-juju compatible with 1.16

Fixes bug lp:1253628 by introducting a workaround:
when the EnvironmentGet API command is not available,
the command runs with direct access to the state db,
as it was in 1.16.

Added a helper in state/api/params/apierror.go, useful
to detect "no such request" errors from the API server.

https://code.launchpad.net/~dimitern/juju-core/203-lp-1253628-upgrade-juju-1.16-compat/+merge/196118

(do not edit description out of merge proposal)

[dimitern, 2013-11-21 14:16:23]

Please take a look.

[jameinel, 2013-11-21 14:26:18]

This LGTM

I think we're going to be using your helper a lot more in the next few days, so
I'd like to see some tests for it.

I would guess we could share more code if we really wanted to, but as this is
compatibility code that we want to remove, it is nice to have it isolated in one
chunk.

conn.State.SetEnvironAgentVersion didn't exist in 1.16 but I think its fine
because it does what we really want to do (rather than setting the whole
EnvironConfig and breaking other stuff).

The only caveat is if there was anything we might have set intentionally (albeit
as a side effect) of the SetEnvironConfig (such as changing the ToolsURL
location).

We need to test this in the wild but we can't until we fix bug #1250154 so I'd
rather see this land for now (I think).

https://codereview.appspot.com/30300043/diff/1/state/api/params/apierror.go
File state/api/params/apierror.go (right):

https://codereview.appspot.com/30300043/diff/1/state/api/params/apierror.go#n...
state/api/params/apierror.go:124: }
We should probably have a test associated with this function.

[dimitern, 2013-11-21 14:43:35]

Please take a look.

https://codereview.appspot.com/30300043/diff/1/state/api/params/apierror.go
File state/api/params/apierror.go (right):

https://codereview.appspot.com/30300043/diff/1/state/api/params/apierror.go#n...
state/api/params/apierror.go:124: }
On 2013/11/21 14:26:19, jameinel wrote:
> We should probably have a test associated with this function.

Done.

[rog, 2013-11-21 15:02:10]

LGTM modulo the below

https://codereview.appspot.com/30300043/diff/20001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/30300043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:127: if err != nil && params.IsNoSuchRequest(err) {
maybe better like this:

if params.IsNoSuchRequest(err) {
    return c.run1dot16()
}
if err != nil {
    return err
}

https://codereview.appspot.com/30300043/diff/20001/state/api/params/apierror.go
File state/api/params/apierror.go (right):

https://codereview.appspot.com/30300043/diff/20001/state/api/params/apierror....
state/api/params/apierror.go:121: func IsNoSuchRequest(err error) bool {
I think this would be better off living inside the rpc package, as it's much
more closely related to that than the juju-specific error codes.

Alternatively rename it to IsNoSuchRPCRequest, but I'd prefer the above.

https://codereview.appspot.com/30300043/diff/20001/state/api/params/apierror....
state/api/params/apierror.go:122: matched, _ := regexp.MatchString("^no such
request .*", err.Error())
if err == nil {
    return false
}

return strings.HasPrefix(err.Error(), "no such request ")

[dimitern, 2013-11-21 15:18:15]

Please take a look.

https://codereview.appspot.com/30300043/diff/20001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/30300043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:127: if err != nil && params.IsNoSuchRequest(err) {
On 2013/11/21 15:02:11, rog wrote:
> maybe better like this:
> 
> if params.IsNoSuchRequest(err) {
>     return c.run1dot16()
> }
> if err != nil {
>     return err
> }

Done.

https://codereview.appspot.com/30300043/diff/20001/state/api/params/apierror.go
File state/api/params/apierror.go (right):

https://codereview.appspot.com/30300043/diff/20001/state/api/params/apierror....
state/api/params/apierror.go:121: func IsNoSuchRequest(err error) bool {
On 2013/11/21 15:02:11, rog wrote:
> I think this would be better off living inside the rpc package, as it's much
> more closely related to that than the juju-specific error codes.
> 
> Alternatively rename it to IsNoSuchRPCRequest, but I'd prefer the above.

Moved to the rpc package.

https://codereview.appspot.com/30300043/diff/20001/state/api/params/apierror....
state/api/params/apierror.go:122: matched, _ := regexp.MatchString("^no such
request .*", err.Error())
On 2013/11/21 15:02:11, rog wrote:
> if err == nil {
>     return false
> }
> 
> return strings.HasPrefix(err.Error(), "no such request ")

Done.

[dimitern, 2013-11-21 15:27:58]

Please take a look.