return error on no environment

return error on no environment

Now, instead of just returning a cryptic filenotfound error, we return a more specific NoEnvError, indicating the environment doesn't exist. When the commands encounter that error, they notify the user, who is told how to create a new environment.

https://code.launchpad.net/~natefinch/juju-core/001-noenv-warning/+merge/178812

(do not edit description out of merge proposal)

[natefinch, 2013-08-06 17:05:42]

Please take a look.

[axw1, 2013-08-07 02:06:34]

On 2013/08/06 17:05:42, nate.finch wrote:
> Please take a look.

LGTM, but I do wonder whether envOpenFailure should be replaced with something a
little more generic. Just an idea, but how about adding this to cmd.Command:

    // Error is invoked after Run, if it returns an error.
    // This method may be used to filter out or otherwise
    // transform errors.
    func Error(err error) error

Then implement EnvCommandBase.Error, and you can remove the envOpenFailure
calls.

[axw1, 2013-08-07 02:07:23]

https://codereview.appspot.com/12546043/diff/1/cmd/juju/environmentcommand.go
File cmd/juju/environmentcommand.go (right):

https://codereview.appspot.com/12546043/diff/1/cmd/juju/environmentcommand.go...
cmd/juju/environmentcommand.go:75: // the caller was using the
Missing the end of the sentence.

[dimitern, 2013-08-07 09:37:29]

Nice! LGTM with a few suggestions below

https://codereview.appspot.com/12546043/diff/1/cmd/juju/environmentcommand.go
File cmd/juju/environmentcommand.go (right):

https://codereview.appspot.com/12546043/diff/1/cmd/juju/environmentcommand.go...
cmd/juju/environmentcommand.go:75: // the caller was using the
On 2013/08/07 02:07:23, axw1 wrote:
> Missing the end of the sentence.

+1

https://codereview.appspot.com/12546043/diff/1/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12546043/diff/1/cmd/juju/init.go#newcode7
cmd/juju/init.go:7: "fmt"
blank line after this, and after gnuflag.

https://codereview.appspot.com/12546043/diff/1/environs/config_test.go
File environs/config_test.go (right):

https://codereview.appspot.com/12546043/diff/1/environs/config_test.go#newcode10
environs/config_test.go:10: . "launchpad.net/gocheck"
While you're at it, can you change this to gc "launchpad.net/gocheck" and it's
references below please?

https://codereview.appspot.com/12546043/diff/1/environs/config_test.go#newcode94
environs/config_test.go:94: if !errors.IsNoEnv(err) {
Perhaps better:
c.Assert(err, gc.FitsTypeOf, &errors.NoEnvError{})
No need for  the message and the if.

[rog, 2013-08-07 15:45:57]

I think nate has a good point actually, and we
can avoid all those envOpenFailure calls
even without implementing an extra Error
entry point to Command, by wrapping
the commands as appropriate.

Something like this, perhaps, in cmd/juju/main.go:

replace all calls to jujucmd.Register with calls to
the registerCommand function below.

func registerCommand(c cmd.Command) cmd.Command {
	if envc, ok := c.(envCommand); ok {
		c = envCommandWrapper{envc}
	}
	jujucmd.Register(c)
}

// envCommand is implemented by commands that
// read an environment file.
type envCommand interface {
	cmd.Command
	EnvironName() string
}

// envCommandWrapper wraps a command that takes
// an environment flag, and makes it print a
// more informative error message when the default
// environment is not found.
type envCommandWrapper struct {
	envCommand
}

func (c envCommandWrapper) Run(ctxt *cmd.Context) error {
	err := c.Command.Run(ctxt)
	if !IsNoEnvError(err) || c.EnvironName() != "" {
		return err
	}
	print details
	return ErrSilent
}

and in environmentcommand.go:

func (c *EnvCommandBase) EnvironName() string { return c.EnvName }

LGTM with the above change and NoEnvError moved
to environs, thanks!

https://codereview.appspot.com/12546043/diff/1/cmd/juju/environmentcommand.go
File cmd/juju/environmentcommand.go (right):

https://codereview.appspot.com/12546043/diff/1/cmd/juju/environmentcommand.go...
cmd/juju/environmentcommand.go:77: if errors.IsNoEnv(err) {
Better to keep the indentation level lower,
and I'm not sure we need to do anything special
when c.EnvName is not nil (the error message
should be sufficient)

For example:

if !errors.IsNoEnv(err) || c.EnvName != "" {
    return err
}
fmt.Fprintln(w, "No juju ....
etc
return cmd.ErrSilent

https://codereview.appspot.com/12546043/diff/1/environs/config_test.go
File environs/config_test.go (right):

https://codereview.appspot.com/12546043/diff/1/environs/config_test.go#newcode94
environs/config_test.go:94: if !errors.IsNoEnv(err) {
On 2013/08/07 09:37:29, dimitern wrote:
> Perhaps better:
> c.Assert(err, gc.FitsTypeOf, &errors.NoEnvError{})
> No need for  the message and the if.

or, better still perhaps:

c.Assert(err, jc.Satisfies, errors.IsNoEnv)

?

https://codereview.appspot.com/12546043/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/12546043/diff/1/errors/errors.go#newcode69
errors/errors.go:69: type NoEnvError struct {
I'm not sure this should live here.
The errors package is about declaring errors that
are potentially produced by many different packages.
This error, on the other hand, is produced by a single
package only. I think it should live there (in environs).

Also, I think that the resulting error message
should reflect the fact that it's a juju environment
that can't be found, rather than reusing the underlying
file-not-found message.

[rog, 2013-08-07 15:47:04]

On 2013/08/07 15:45:57, rog wrote:
> I think nate has a good point actually, and we

oops, s/nate/andrew/

[natefinch, 2013-08-07 20:25:53]

> I think [Andrew] has a good point actually, and we
> can avoid all those envOpenFailure calls
> even without implementing an extra Error
> entry point to Command, by wrapping
> the commands as appropriate.
> 
> Something like this, perhaps, in cmd/juju/main.go:
> 
> replace all calls to jujucmd.Register with calls to
> the registerCommand function below.
> 
> func registerCommand(c cmd.Command) cmd.Command {
> 	if envc, ok := c.(envCommand); ok {
> 		c = envCommandWrapper{envc}
> 	}
> 	jujucmd.Register(c)
> }

Sounds good, will do.



> https://codereview.appspot.com/12546043/diff/1/environs/config_test.go
> File environs/config_test.go (right):
> 
>
https://codereview.appspot.com/12546043/diff/1/environs/config_test.go#newcode94
> environs/config_test.go:94: if !errors.IsNoEnv(err) {
> On 2013/08/07 09:37:29, dimitern wrote:
> > Perhaps better:
> > c.Assert(err, gc.FitsTypeOf, &errors.NoEnvError{})
> > No need for  the message and the if.
> 
> or, better still perhaps:
> 
> c.Assert(err, jc.Satisfies, errors.IsNoEnv)
 

Yep, I wrote that before I found checkers.Satisfies.... and I guess I forgot to
go back and fix it.

> https://codereview.appspot.com/12546043/diff/1/errors/errors.go
> File errors/errors.go (right):
> 
> https://codereview.appspot.com/12546043/diff/1/errors/errors.go#newcode69
> errors/errors.go:69: type NoEnvError struct {
> I'm not sure this should live here.
> The errors package is about declaring errors that
> are potentially produced by many different packages.
> This error, on the other hand, is produced by a single
> package only. I think it should live there (in environs).
> 
> Also, I think that the resulting error message
> should reflect the fact that it's a juju environment
> that can't be found, rather than reusing the underlying
> file-not-found message.

Yeah, the only reason I put it there is because it causes an import cycle
(actually multiple, IIRC) when it's in environs. I'll see if there's a way
around that, or if I can put it somewhere more appropriate than errors.go.

[natefinch, 2013-08-08 16:53:38]

Please take a look.

[dimitern, 2013-08-08 17:16:52]

LGTM, if you revert the changes to files which do not need Run(ctx ..) and (err
error). Also please use gc as an alias for gocheck.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addrelation.go
File cmd/juju/addrelation.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addrelation.go#ne...
cmd/juju/addrelation.go:37: func (c *AddRelationCommand) Run(ctx *cmd.Context)
(err error) {
Please remove the named return value, it's unnecessary (here and in all
subsequent cases). Also, the ctx is not used, so leaving it to be _ is ok. Can
you just revert these changes?

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/config_test.go#ne...
cmd/juju/config_test.go:10: "launchpad.net/gocheck"
gc "..." please, and change the rest to use gc. prefix.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/init_test.go
File cmd/juju/init_test.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/init_test.go#newc...
cmd/juju/init_test.go:11: "launchpad.net/gocheck"
Here as well: gc ".."

https://codereview.appspot.com/12546043/diff/13001/environs/config_test.go
File environs/config_test.go (right):

https://codereview.appspot.com/12546043/diff/13001/environs/config_test.go#ne...
environs/config_test.go:10: "launchpad.net/gocheck"
And here..

[rog, 2013-08-08 17:25:40]

LGTM modulo trivials below.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:70: func (c *AddMachineCommand) Run(ctx *cmd.Context)
error {
These can probably revert to the original. It's quite nice to see immediately
that the context isn't used.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addrelation.go
File cmd/juju/addrelation.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addrelation.go#ne...
cmd/juju/addrelation.go:37: func (c *AddRelationCommand) Run(ctx *cmd.Context)
(err error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addunit.go
File cmd/juju/addunit.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/addunit.go#newcode91
cmd/juju/addunit.go:91: func (c *AddUnitCommand) Run(ctx *cmd.Context) (err
error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:60: func (c *BootstrapCommand) Run(ctx *cmd.Context) (err
error) {
No need to declare err in the return values here.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/config_test.go#ne...
cmd/juju/config_test.go:10: "launchpad.net/gocheck"
The convention is to import gocheck as gc.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/deploy.go#newcode99
cmd/juju/deploy.go:99: func (c *DeployCommand) Run(ctx *cmd.Context) (err error)
{
No need to declare err here.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyenvironmen...
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyenvironmen...
cmd/juju/destroyenvironment.go:23: func (c *DestroyEnvironmentCommand) Run(ctx
*cmd.Context) error {
s/ctx/_/

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroymachine.go
File cmd/juju/destroymachine.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroymachine.go...
cmd/juju/destroymachine.go:43: func (c *DestroyMachineCommand) Run(ctx
*cmd.Context) (err error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyrelation.go
File cmd/juju/destroyrelation.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyrelation.g...
cmd/juju/destroyrelation.go:38: func (c *DestroyRelationCommand) Run(ctx
*cmd.Context) (err error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyservice.go
File cmd/juju/destroyservice.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyservice.go...
cmd/juju/destroyservice.go:42: func (c *DestroyServiceCommand) Run(ctx
*cmd.Context) error {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyunit.go
File cmd/juju/destroyunit.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/destroyunit.go#ne...
cmd/juju/destroyunit.go:47: func (c *DestroyUnitCommand) Run(ctx *cmd.Context)
(err error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/environment.go#ne...
cmd/juju/environment.go:131: func (c *SetEnvironmentCommand) Run(ctx
*cmd.Context) (err error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/expose.go
File cmd/juju/expose.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/expose.go#newcode39
cmd/juju/expose.go:39: func (c *ExposeCommand) Run(ctx *cmd.Context) (err error)
{
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/get.go
File cmd/juju/get.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/get.go#newcode51
cmd/juju/get.go:51: func (c *GetCommand) Run(ctx *cmd.Context) (err error) {
ditto

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/init_test.go
File cmd/juju/init_test.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/init_test.go#newc...
cmd/juju/init_test.go:11: "launchpad.net/gocheck"
gc "launchpad.net/gocheck"

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/main.go#newcode126
cmd/juju/main.go:126: err := c.envCmd.Run(ctx)
if !environs.IsNoEnv(err) || c.EnvironName() != "" {
    return err
}
fmt.Fprintln etc

?
just to keep the indentation level down, particularly with those long lines.

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/main.go#newcode129
cmd/juju/main.go:129: fmt.Fprintln(ctx.Stderr, err.Error())
s/.Error()//

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/resolved.go
File cmd/juju/resolved.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/resolved.go#newco...
cmd/juju/resolved.go:50: func (c *ResolvedCommand) Run(ctx *cmd.Context) (err
error) {
same

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/unexpose.go
File cmd/juju/unexpose.go (right):

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/unexpose.go#newco...
cmd/juju/unexpose.go:39: func (c *UnexposeCommand) Run(ctx *cmd.Context) (err
error) {
same

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

https://codereview.appspot.com/12546043/diff/13001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:100: func (c *UpgradeJujuCommand) Run(ctx *cmd.Context)
(err error) {
same

https://codereview.appspot.com/12546043/diff/13001/environs/errors.go
File environs/errors.go (right):

https://codereview.appspot.com/12546043/diff/13001/environs/errors.go#newcode12
environs/errors.go:12: func IsNoEnv(err error) bool {
This could probably be declared alongside ReadEnvirons, the only place it's
used.

[natefinch, 2013-08-08 20:37:47]

Please take a look.

[natefinch, 2013-08-08 20:39:42]

I did everything as asked, except for Roger's change to main.go... I
intentionally am keeping the function open ended so that you can add more error
handling for different errors after the code I've written. If I returned early
as suggested, then adding new error handling later in the function would require
refactoring that code.

Apologies for the spurious changes left in in the last review... I'm new to
bazaar and it took me a while to figure out how to show the changes on the
branch versus the parent branch.

[jameinel, 2013-08-13 13:22:35]

what is this still waiting on? Is it just that Nate wants people to confirm that
he did the changes they requested?

[jameinel, 2013-08-13 13:56:34]

On 2013/08/13 13:22:35, jameinel wrote:
> what is this still waiting on? Is it just that Nate wants people to confirm
that
> he did the changes they requested?

The reviews were "LGTM with these changes" which generally means that if you
make those changes, you can mark the request Approved and land it.
You only need to seek further reviews if you feel you didn't understand/didn't
quite do what was requested, and you want to continue the conversation.

So I think you can just land this, but I didn't do an actual review myself.