Merge into trunk : switch-show-user : Code : juju-core

Status:	Needs review
Proposed branch:	lp:~waigani/juju-core/switch-show-user
Merge into:	lp:~go-bot/juju-core/trunk
Diff against target:	363 lines (+234/-33) 2 files modified cmd/juju/switch.go (+165/-33) cmd/juju/switch_test.go (+69/-0)
To merge this branch:	bzr merge lp:~waigani/juju-core/switch-show-user
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2014-05-25	Pending
Review via email: mp+220868@code.launchpad.net

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-25:

#

Download full text (6.9 KiB)

Reviewers: mp+220868_code.launchpad.net,

Message:
Please take a look.

Description:
Add user infor to switch

Append current user's username to current environment
info printed out by `$ juju switch`.

https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/92610043/

Affected files (+91, -5 lines):
   A [revision details]
   M cmd/juju/switch.go
   M cmd/juju/switch_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
+New revision: <email address hidden>

Index: cmd/juju/switch.go
=== modified file 'cmd/juju/switch.go'
--- cmd/juju/switch.go 2014-05-09 13:24:50 +0000
+++ cmd/juju/switch.go 2014-05-25 01:07:06 +0000
@@ -4,16 +4,17 @@
package main

  import (
- "errors"
   "fmt"
   "os"
   "sort"

+ "github.com/juju/errors"
"launchpad.net/gnuflag"

   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/cmd/envcmd"
   "launchpad.net/juju-core/environs"
+ "launchpad.net/juju-core/environs/configstore"
  )

  type SwitchCommand struct {
@@ -62,6 +63,26 @@
   return false
  }

+// printEnvInfo appends the current user to the current environment and
prints
+// both to Stdout when `$ juju switch` is called
+func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
error) {
+ userInfo := ""
+ store, err := configstore.Default()
+ if err != nil {
+ return err
+ }
+ info, err := store.ReadInfo(envName)
+ if err != nil {
+ if !errors.IsNotFound(err) {
+ return err
+ }
+ } else {
+ userInfo = fmt.Sprintf("user: %s\n", info.APICredentials().User)
+ }
+ fmt.Fprint(ctx.Stdout, msg+userInfo)
+ return nil
+}
+
  func (c *SwitchCommand) Run(ctx *cmd.Context) error {
   // Switch is an alternative way of dealing with environments than using
   // the JUJU_ENV environment setting, and as such, doesn't play too well.
@@ -90,7 +111,10 @@
   jujuEnv := os.Getenv("JUJU_ENV")
   if jujuEnv != "" {
    if c.EnvName == "" {
- fmt.Fprintf(ctx.Stdout, "%s\n", jujuEnv)
+ msg := fmt.Sprintf("%s\n", jujuEnv)
+ if err := printEnvInfo(ctx, msg, jujuEnv); err != nil {
+ return err
+ }
     return nil
    } else {
     return fmt.Errorf("cannot switch when JUJU_ENV is overriding the
environment (set to %q)", jujuEnv)
@@ -109,7 +133,10 @@
    return errors.New("no currently specified environment")
   case c.EnvName == "":
    // Simply print the current environment.
- fmt.Fprintf(ctx.Stdout, "%s\n", currentEnv)
+ msg := fmt.Sprintf("%s\n", currentEnv)
+ if err := printEnvInfo(ctx, msg, currentEnv); err != nil {
+ return err
+ }
   default:
    // Switch the environment.
    if !validEnvironmentName(c.EnvName, names) {
@@ -119,9 +146,15 @@
     return err
    }
    if currentEnv == "" {
- fmt.Fprintf(ctx.Stdout, "-> %s\n", c.EnvName)
+ msg := fmt.Sprintf("-> %s\n", c.EnvName)
+ if err := printEnvInfo(ctx, msg, c.EnvName); err != nil {
+ return err
+ }
    } else {
- fmt.Fprin...

Reviewers: mp+220868_code.launchpad.net,

Message:
Please take a look.

Description:
Add user infor to switch

Append current user's username to current environment
info printed out by `$ juju switch`.

https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/92610043/

Affected files (+91, -5 lines):
   A [revision details]
   M cmd/juju/switch.go
   M cmd/juju/switch_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
+New revision: jesse.meek@canonical.com-20140525010706-80ir2jmt3rww66a0

Index: cmd/juju/switch.go
=== modified file 'cmd/juju/switch.go'
--- cmd/juju/switch.go	2014-05-09 13:24:50 +0000
+++ cmd/juju/switch.go	2014-05-25 01:07:06 +0000
@@ -4,16 +4,17 @@
  package main

import (
-	"errors"
  	"fmt"
  	"os"
  	"sort"

+	"github.com/juju/errors"
  	"launchpad.net/gnuflag"

"launchpad.net/juju-core/cmd"
  	"launchpad.net/juju-core/cmd/envcmd"
  	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/configstore"
  )

type SwitchCommand struct {
@@ -62,6 +63,26 @@
  	return false
  }

+// printEnvInfo appends the current user to the current environment and  
prints
+// both to Stdout when `$ juju switch` is called
+func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err  
error) {
+	userInfo := ""
+	store, err := configstore.Default()
+	if err != nil {
+		return err
+	}
+	info, err := store.ReadInfo(envName)
+	if err != nil {
+		if !errors.IsNotFound(err) {
+			return err
+		}
+	} else {
+		userInfo = fmt.Sprintf("user: %s\n", info.APICredentials().User)
+	}
+	fmt.Fprint(ctx.Stdout, msg+userInfo)
+	return nil
+}
+
  func (c *SwitchCommand) Run(ctx *cmd.Context) error {
  	// Switch is an alternative way of dealing with environments than using
  	// the JUJU_ENV environment setting, and as such, doesn't play too well.
@@ -90,7 +111,10 @@
  	jujuEnv := os.Getenv("JUJU_ENV")
  	if jujuEnv != "" {
  		if c.EnvName == "" {
-			fmt.Fprintf(ctx.Stdout, "%s\n", jujuEnv)
+			msg := fmt.Sprintf("%s\n", jujuEnv)
+			if err := printEnvInfo(ctx, msg, jujuEnv); err != nil {
+				return err
+			}
  			return nil
  		} else {
  			return fmt.Errorf("cannot switch when JUJU_ENV is overriding the  
environment (set to %q)", jujuEnv)
@@ -109,7 +133,10 @@
  		return errors.New("no currently specified environment")
  	case c.EnvName == "":
  		// Simply print the current environment.
-		fmt.Fprintf(ctx.Stdout, "%s\n", currentEnv)
+		msg := fmt.Sprintf("%s\n", currentEnv)
+		if err := printEnvInfo(ctx, msg, currentEnv); err != nil {
+			return err
+		}
  	default:
  		// Switch the environment.
  		if !validEnvironmentName(c.EnvName, names) {
@@ -119,9 +146,15 @@
  			return err
  		}
  		if currentEnv == "" {
-			fmt.Fprintf(ctx.Stdout, "-> %s\n", c.EnvName)
+			msg := fmt.Sprintf("-> %s\n", c.EnvName)
+			if err := printEnvInfo(ctx, msg, c.EnvName); err != nil {
+				return err
+			}
  		} else {
-			fmt.Fprintf(ctx.Stdout, "%s -> %s\n", currentEnv, c.EnvName)
+			msg := fmt.Sprintf("%s -> %s\n", currentEnv, c.EnvName)
+			if err := printEnvInfo(ctx, msg, c.EnvName); err != nil {
+				return err
+			}
  		}
  	}
  	return nil

Index: cmd/juju/switch_test.go
=== modified file 'cmd/juju/switch_test.go'
--- cmd/juju/switch_test.go	2014-05-22 06:03:06 +0000
+++ cmd/juju/switch_test.go	2014-05-25 01:07:06 +0000
@@ -9,6 +9,7 @@
  	gc "launchpad.net/gocheck"

"launchpad.net/juju-core/cmd/envcmd"
+	"launchpad.net/juju-core/environs/configstore"
  	_ "launchpad.net/juju-core/juju"
  	"launchpad.net/juju-core/testing"
  )
@@ -19,6 +20,23 @@

var _ = gc.Suite(&SwitchSimpleSuite{})

+var testCreds = configstore.APICredentials{
+	User:     "joblow",
+	Password: "baloney",
+}
+
+func patchEnvWithUser(c *gc.C, envName string) {
+	testing.WriteEnvironments(c, testing.MultipleEnvConfig)
+
+	// Patch APICredentials so that <envName>.jenv file is avaliable for
+	// switch to read the envirionment user from.
+	store, err := configstore.Default()
+	c.Assert(err, gc.IsNil)
+	info, err := store.CreateInfo(envName)
+	info.SetAPICredentials(testCreds)
+	info.Write()
+}
+
  func (*SwitchSimpleSuite) TestNoEnvironment(c *gc.C) {
  	envPath := testing.HomePath(".juju", "environments.yaml")
  	err := os.Remove(envPath)
@@ -40,6 +58,13 @@
  	c.Assert(testing.Stdout(context), gc.Equals, "erewhemos\n")
  }

+func (s *SwitchSimpleSuite) TestShowsDefaultAndUser(c *gc.C) {
+	patchEnvWithUser(c, "erewhemos")
+	context, err := testing.RunCommand(c, &SwitchCommand{})
+	c.Assert(err, gc.IsNil)
+	c.Assert(testing.Stdout(context),  
gc.Equals, "erewhemos\nuser: "+testCreds.User+"\n")
+}
+
  func (s *SwitchSimpleSuite) TestCurrentEnvironmentHasPrecidence(c *gc.C) {
  	testing.WriteEnvironments(c, testing.MultipleEnvConfig)
  	s.FakeHomeSuite.Home.AddFiles(c,  
testing.TestFile{".juju/current-environment", "fubar"})
@@ -48,6 +73,14 @@
  	c.Assert(testing.Stdout(context), gc.Equals, "fubar\n")
  }

+func (s *SwitchSimpleSuite)  
TestCurrentEnvironmentHasPrecidenceAndShowUser(c *gc.C) {
+	patchEnvWithUser(c, "fubar")
+	s.FakeHomeSuite.Home.AddFiles(c,  
testing.TestFile{".juju/current-environment", "fubar"})
+	context, err := testing.RunCommand(c, &SwitchCommand{})
+	c.Assert(err, gc.IsNil)
+	c.Assert(testing.Stdout(context),  
gc.Equals, "fubar\nuser: "+testCreds.User+"\n")
+}
+
  func (*SwitchSimpleSuite) TestShowsJujuEnv(c *gc.C) {
  	testing.WriteEnvironments(c, testing.MultipleEnvConfig)
  	os.Setenv("JUJU_ENV", "using-env")
@@ -56,6 +89,16 @@
  	c.Assert(testing.Stdout(context), gc.Equals, "using-env\n")
  }

+func (*SwitchSimpleSuite) TestShowsJujuEnvAndUser(c *gc.C) {
+	// testing.WriteEnvironments(c, testing.MultipleEnvConfig)
+	envName := "using-env"
+	os.Setenv("JUJU_ENV", envName)
+	patchEnvWithUser(c, envName)
+	context, err := testing.RunCommand(c, &SwitchCommand{})
+	c.Assert(err, gc.IsNil)
+	c.Assert(testing.Stdout(context),  
gc.Equals, "using-env\nuser: "+testCreds.User+"\n")
+}
+
  func (s *SwitchSimpleSuite) TestJujuEnvOverCurrentEnvironment(c *gc.C) {
  	testing.WriteEnvironments(c, testing.MultipleEnvConfig)
  	s.FakeHomeSuite.Home.AddFiles(c,  
testing.TestFile{".juju/current-environment", "fubar"})
@@ -73,6 +116,14 @@
  	c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
  }

+func (*SwitchSimpleSuite) TestSettingWritesFileAndShowUser(c *gc.C) {
+	patchEnvWithUser(c, "erewhemos-2")
+	context, err := testing.RunCommand(c, &SwitchCommand{}, "erewhemos-2")
+	c.Assert(err, gc.IsNil)
+	c.Assert(testing.Stdout(context), gc.Equals, "erewhemos ->  
erewhemos-2\nuser: "+testCreds.User+"\n")
+	c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
+}
+
  func (*SwitchSimpleSuite) TestSettingToUnknown(c *gc.C) {
  	testing.WriteEnvironments(c, testing.MultipleEnvConfig)
  	_, err := testing.RunCommand(c, &SwitchCommand{}, "unknown")

Reply

Revision history for this message

Menno Finlay-Smits (menno.smits) wrote on 2014-05-25:

#

LGTM. Nitpicks only.

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

https://codereview.appspot.com/92610043/diff/1/cmd/juju/switch.go#newcode84
cmd/juju/switch.go:84: }
This might be cleaner if you extracted the code to retrieve the current
user to it's own function that printEnvInfo calls.

Personally, I'd also move printEnvInfo underneath Run. It's considered
good practice to have higher level functions towards the top of a source
file and to have the lower level functions they use underneath. It makes
the code easier for other developers to read because the lower level
functionality can be easily ignored - or at least doesn't have to be
digested up front - in order to get an understanding of what the code is
doing and how it hangs together.

https://codereview.appspot.com/92610043/diff/1/cmd/juju/switch.go#newcode153
cmd/juju/switch.go:153: } else {
The 2 printEnvInfo calls here could be reduced to one by calling it
after the "if currentEnv" block.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2014-05-25:

#

Download full text (9.0 KiB)

Implementation looks fine in general (+1 to Menno's remarks) but...

Having juju switch print this seems a bit odd as it doesn't have anything
else to do with users, does it? Is there a "juju user" command? If not we
could make one. BTW this change may potentially break backwards
compatibility with sh scripts I think.
On 25 May 2014 02:15, "Jesse Meek" <email address hidden> wrote:

> Reviewers: mp+220868_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> Add user infor to switch
>
> Append current user's username to current environment
> info printed out by `$ juju switch`.
>
>
> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/92610043/
>
> Affected files (+91, -5 lines):
> A [revision details]
> M cmd/juju/switch.go
> M cmd/juju/switch_test.go
>
>
> Index: [revision details]
> === added file '[revision details]'
> --- [revision details] 2012-01-01 00:00:00 +0000
> +++ [revision details] 2012-01-01 00:00:00 +0000
> @@ -0,0 +1,2 @@
> +Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
> +New revision: <email address hidden>
>
> Index: cmd/juju/switch.go
> === modified file 'cmd/juju/switch.go'
> --- cmd/juju/switch.go 2014-05-09 13:24:50 +0000
> +++ cmd/juju/switch.go 2014-05-25 01:07:06 +0000
> @@ -4,16 +4,17 @@
> package main
>
> import (
> - "errors"
> "fmt"
> "os"
> "sort"
>
> + "github.com/juju/errors"
> "launchpad.net/gnuflag"
>
> "launchpad.net/juju-core/cmd"
> "launchpad.net/juju-core/cmd/envcmd"
> "launchpad.net/juju-core/environs"
> + "launchpad.net/juju-core/environs/configstore"
> )
>
> type SwitchCommand struct {
> @@ -62,6 +63,26 @@
> return false
> }
>
> +// printEnvInfo appends the current user to the current environment and
> prints
> +// both to Stdout when `$ juju switch` is called
> +func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
> error) {
> + userInfo := ""
> + store, err := configstore.Default()
> + if err != nil {
> + return err
> + }
> + info, err := store.ReadInfo(envName)
> + if err != nil {
> + if !errors.IsNotFound(err) {
> + return err
> + }
> + } else {
> + userInfo = fmt.Sprintf("user: %s\n",
> info.APICredentials().User)
> + }
> + fmt.Fprint(ctx.Stdout, msg+userInfo)
> + return nil
> +}
> +
> func (c *SwitchCommand) Run(ctx *cmd.Context) error {
> // Switch is an alternative way of dealing with environments than
> using
> // the JUJU_ENV environment setting, and as such, doesn't play too
> well.
> @@ -90,7 +111,10 @@
> jujuEnv := os.Getenv("JUJU_ENV")
> if jujuEnv != "" {
> if c.EnvName == "" {
> - fmt.Fprintf(ctx.Stdout, "%s\n", jujuEnv)
> + msg := fmt.Sprintf("%s\n", jujuEnv)
> + if err := printEnvInfo(ctx, msg, jujuEnv); err !=
> nil ...

Implementation looks fine in general (+1 to Menno's remarks) but...

Having juju switch print this seems a bit odd as it doesn't have anything
else to do with users, does it? Is there a "juju user" command? If not we
could make one. BTW this change may potentially break backwards
compatibility with sh scripts I think.
On 25 May 2014 02:15, "Jesse Meek" <jesse.meek@canonical.com> wrote:

> Reviewers: mp+220868_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> Add user infor to switch
>
> Append current user's username to current environment
> info printed out by `$ juju switch`.
>
>
> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/92610043/
>
> Affected files (+91, -5 lines):
>    A [revision details]
>    M cmd/juju/switch.go
>    M cmd/juju/switch_test.go
>
>
> Index: [revision details]
> === added file '[revision details]'
> --- [revision details]  2012-01-01 00:00:00 +0000
> +++ [revision details]  2012-01-01 00:00:00 +0000
> @@ -0,0 +1,2 @@
> +Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
> +New revision: jesse.meek@canonical.com-20140525010706-80ir2jmt3rww66a0
>
> Index: cmd/juju/switch.go
> === modified file 'cmd/juju/switch.go'
> --- cmd/juju/switch.go  2014-05-09 13:24:50 +0000
> +++ cmd/juju/switch.go  2014-05-25 01:07:06 +0000
> @@ -4,16 +4,17 @@
>   package main
>
>   import (
> -       "errors"
>         "fmt"
>         "os"
>         "sort"
>
> +       "github.com/juju/errors"
>         "launchpad.net/gnuflag"
>
>         "launchpad.net/juju-core/cmd"
>         "launchpad.net/juju-core/cmd/envcmd"
>         "launchpad.net/juju-core/environs"
> +       "launchpad.net/juju-core/environs/configstore"
>   )
>
>   type SwitchCommand struct {
> @@ -62,6 +63,26 @@
>         return false
>   }
>
> +// printEnvInfo appends the current user to the current environment and
> prints
> +// both to Stdout when `$ juju switch` is called
> +func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
> error) {
> +       userInfo := ""
> +       store, err := configstore.Default()
> +       if err != nil {
> +               return err
> +       }
> +       info, err := store.ReadInfo(envName)
> +       if err != nil {
> +               if !errors.IsNotFound(err) {
> +                       return err
> +               }
> +       } else {
> +               userInfo = fmt.Sprintf("user: %s\n",
> info.APICredentials().User)
> +       }
> +       fmt.Fprint(ctx.Stdout, msg+userInfo)
> +       return nil
> +}
> +
>   func (c *SwitchCommand) Run(ctx *cmd.Context) error {
>         // Switch is an alternative way of dealing with environments than
> using
>         // the JUJU_ENV environment setting, and as such, doesn't play too
> well.
> @@ -90,7 +111,10 @@
>         jujuEnv := os.Getenv("JUJU_ENV")
>         if jujuEnv != "" {
>                 if c.EnvName == "" {
> -                       fmt.Fprintf(ctx.Stdout, "%s\n", jujuEnv)
> +                       msg := fmt.Sprintf("%s\n", jujuEnv)
> +                       if err := printEnvInfo(ctx, msg, jujuEnv); err !=
> nil {
> +                               return err
> +                       }
>                         return nil
>                 } else {
>                         return fmt.Errorf("cannot switch when JUJU_ENV is
> overriding the
> environment (set to %q)", jujuEnv)
> @@ -109,7 +133,10 @@
>                 return errors.New("no currently specified environment")
>         case c.EnvName == "":
>                 // Simply print the current environment.
> -               fmt.Fprintf(ctx.Stdout, "%s\n", currentEnv)
> +               msg := fmt.Sprintf("%s\n", currentEnv)
> +               if err := printEnvInfo(ctx, msg, currentEnv); err != nil {
> +                       return err
> +               }
>         default:
>                 // Switch the environment.
>                 if !validEnvironmentName(c.EnvName, names) {
> @@ -119,9 +146,15 @@
>                         return err
>                 }
>                 if currentEnv == "" {
> -                       fmt.Fprintf(ctx.Stdout, "-> %s\n", c.EnvName)
> +                       msg := fmt.Sprintf("-> %s\n", c.EnvName)
> +                       if err := printEnvInfo(ctx, msg, c.EnvName); err
> != nil {
> +                               return err
> +                       }
>                 } else {
> -                       fmt.Fprintf(ctx.Stdout, "%s -> %s\n", currentEnv,
> c.EnvName)
> +                       msg := fmt.Sprintf("%s -> %s\n", currentEnv,
> c.EnvName)
> +                       if err := printEnvInfo(ctx, msg, c.EnvName); err
> != nil {
> +                               return err
> +                       }
>                 }
>         }
>         return nil
>
>
> Index: cmd/juju/switch_test.go
> === modified file 'cmd/juju/switch_test.go'
> --- cmd/juju/switch_test.go     2014-05-22 06:03:06 +0000
> +++ cmd/juju/switch_test.go     2014-05-25 01:07:06 +0000
> @@ -9,6 +9,7 @@
>         gc "launchpad.net/gocheck"
>
>         "launchpad.net/juju-core/cmd/envcmd"
> +       "launchpad.net/juju-core/environs/configstore"
>         _ "launchpad.net/juju-core/juju"
>         "launchpad.net/juju-core/testing"
>   )
> @@ -19,6 +20,23 @@
>
>   var _ = gc.Suite(&SwitchSimpleSuite{})
>
> +var testCreds = configstore.APICredentials{
> +       User:     "joblow",
> +       Password: "baloney",
> +}
> +
> +func patchEnvWithUser(c *gc.C, envName string) {
> +       testing.WriteEnvironments(c, testing.MultipleEnvConfig)
> +
> +       // Patch APICredentials so that <envName>.jenv file is avaliable
> for
> +       // switch to read the envirionment user from.
> +       store, err := configstore.Default()
> +       c.Assert(err, gc.IsNil)
> +       info, err := store.CreateInfo(envName)
> +       info.SetAPICredentials(testCreds)
> +       info.Write()
> +}
> +
>   func (*SwitchSimpleSuite) TestNoEnvironment(c *gc.C) {
>         envPath := testing.HomePath(".juju", "environments.yaml")
>         err := os.Remove(envPath)
> @@ -40,6 +58,13 @@
>         c.Assert(testing.Stdout(context), gc.Equals, "erewhemos\n")
>   }
>
> +func (s *SwitchSimpleSuite) TestShowsDefaultAndUser(c *gc.C) {
> +       patchEnvWithUser(c, "erewhemos")
> +       context, err := testing.RunCommand(c, &SwitchCommand{})
> +       c.Assert(err, gc.IsNil)
> +       c.Assert(testing.Stdout(context),
> gc.Equals, "erewhemos\nuser: "+testCreds.User+"\n")
> +}
> +
>   func (s *SwitchSimpleSuite) TestCurrentEnvironmentHasPrecidence(c *gc.C)
> {
>         testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>         s.FakeHomeSuite.Home.AddFiles(c,
> testing.TestFile{".juju/current-environment", "fubar"})
> @@ -48,6 +73,14 @@
>         c.Assert(testing.Stdout(context), gc.Equals, "fubar\n")
>   }
>
> +func (s *SwitchSimpleSuite)
> TestCurrentEnvironmentHasPrecidenceAndShowUser(c *gc.C) {
> +       patchEnvWithUser(c, "fubar")
> +       s.FakeHomeSuite.Home.AddFiles(c,
> testing.TestFile{".juju/current-environment", "fubar"})
> +       context, err := testing.RunCommand(c, &SwitchCommand{})
> +       c.Assert(err, gc.IsNil)
> +       c.Assert(testing.Stdout(context),
> gc.Equals, "fubar\nuser: "+testCreds.User+"\n")
> +}
> +
>   func (*SwitchSimpleSuite) TestShowsJujuEnv(c *gc.C) {
>         testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>         os.Setenv("JUJU_ENV", "using-env")
> @@ -56,6 +89,16 @@
>         c.Assert(testing.Stdout(context), gc.Equals, "using-env\n")
>   }
>
> +func (*SwitchSimpleSuite) TestShowsJujuEnvAndUser(c *gc.C) {
> +       // testing.WriteEnvironments(c, testing.MultipleEnvConfig)
> +       envName := "using-env"
> +       os.Setenv("JUJU_ENV", envName)
> +       patchEnvWithUser(c, envName)
> +       context, err := testing.RunCommand(c, &SwitchCommand{})
> +       c.Assert(err, gc.IsNil)
> +       c.Assert(testing.Stdout(context),
> gc.Equals, "using-env\nuser: "+testCreds.User+"\n")
> +}
> +
>   func (s *SwitchSimpleSuite) TestJujuEnvOverCurrentEnvironment(c *gc.C) {
>         testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>         s.FakeHomeSuite.Home.AddFiles(c,
> testing.TestFile{".juju/current-environment", "fubar"})
> @@ -73,6 +116,14 @@
>         c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
>   }
>
> +func (*SwitchSimpleSuite) TestSettingWritesFileAndShowUser(c *gc.C) {
> +       patchEnvWithUser(c, "erewhemos-2")
> +       context, err := testing.RunCommand(c, &SwitchCommand{},
> "erewhemos-2")
> +       c.Assert(err, gc.IsNil)
> +       c.Assert(testing.Stdout(context), gc.Equals, "erewhemos ->
> erewhemos-2\nuser: "+testCreds.User+"\n")
> +       c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
> +}
> +
>   func (*SwitchSimpleSuite) TestSettingToUnknown(c *gc.C) {
>         testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>         _, err := testing.RunCommand(c, &SwitchCommand{}, "unknown")
>
>
>
>
>
> --
>
> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
> Your team juju hackers is requested to review the proposed merge of
> lp:~waigani/juju-core/switch-show-user into lp:juju-core.
>

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-25:

#

Download full text (9.6 KiB)

Hi Rog,

Yeah, I also think it would make more sense for a subcommand of juju
user to show the user info. I will bring that up in stand up today.

BTW, I want your phone! Where can I find one?

Cheers,
Jess

On 26/05/14 10:21, Roger Peppe wrote:
> Implementation looks fine in general (+1 to Menno's remarks) but.
> Having juju switch print this seems a bit odd as it doesn't have anything
> else to do with users, does it? Is there a "juju user" command? If not we
> could make one. BTW this change may potentially break backwards
> compatibility with sh scripts I think.
> On 25 May 2014 02:15, "Jesse Meek" <email address hidden> wrote:
>
>> Reviewers: mp+220868_code.launchpad.net,
>>
>> Message:
>> Please take a look.
>>
>> Description:
>> Add user infor to switch
>>
>> Append current user's username to current environment
>> info printed out by `$ juju switch`.
>>
>>
>> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>>
>> (do not edit description out of merge proposal)
>>
>>
>> Please review this at https://codereview.appspot.com/92610043/
>>
>> Affected files (+91, -5 lines):
>> A [revision details]
>> M cmd/juju/switch.go
>> M cmd/juju/switch_test.go
>>
>>
>> Index: [revision details]
>> === added file '[revision details]'
>> --- [revision details] 2012-01-01 00:00:00 +0000
>> +++ [revision details] 2012-01-01 00:00:00 +0000
>> @@ -0,0 +1,2 @@
>> +Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
>> +New revision: <email address hidden>
>>
>> Index: cmd/juju/switch.go
>> === modified file 'cmd/juju/switch.go'
>> --- cmd/juju/switch.go 2014-05-09 13:24:50 +0000
>> +++ cmd/juju/switch.go 2014-05-25 01:07:06 +0000
>> @@ -4,16 +4,17 @@
>> package main
>>
>> import (
>> - "errors"
>> "fmt"
>> "os"
>> "sort"
>>
>> + "github.com/juju/errors"
>> "launchpad.net/gnuflag"
>>
>> "launchpad.net/juju-core/cmd"
>> "launchpad.net/juju-core/cmd/envcmd"
>> "launchpad.net/juju-core/environs"
>> + "launchpad.net/juju-core/environs/configstore"
>> )
>>
>> type SwitchCommand struct {
>> @@ -62,6 +63,26 @@
>> return false
>> }
>>
>> +// printEnvInfo appends the current user to the current environment and
>> prints
>> +// both to Stdout when `$ juju switch` is called
>> +func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
>> error) {
>> + userInfo := ""
>> + store, err := configstore.Default()
>> + if err != nil {
>> + return err
>> + }
>> + info, err := store.ReadInfo(envName)
>> + if err != nil {
>> + if !errors.IsNotFound(err) {
>> + return err
>> + }
>> + } else {
>> + userInfo = fmt.Sprintf("user: %s\n",
>> info.APICredentials().User)
>> + }
>> + fmt.Fprint(ctx.Stdout, msg+userInfo)
>> + return nil
>> +}
>> +
>> func (c *SwitchCommand) Run(ctx *cmd.Context) error {
>> // Switch is an alternative way of dealing with environments than
>> using
>> // the JUJU_ENV environment setting, and as such, doesn'...

Hi Rog,

Yeah, I also think it would make more sense for a subcommand of juju 
user to show the user info. I will bring that up in stand up today.

BTW, I want your phone! Where can I find one?

Cheers,
Jess

On 26/05/14 10:21, Roger Peppe wrote:
> Implementation looks fine in general (+1 to Menno's remarks) but.
> Having juju switch print this seems a bit odd as it doesn't have anything
> else to do with users, does it? Is there a "juju user" command? If not we
> could make one. BTW this change may potentially break backwards
> compatibility with sh scripts I think.
> On 25 May 2014 02:15, "Jesse Meek" <jesse.meek@canonical.com> wrote:
>
>> Reviewers: mp+220868_code.launchpad.net,
>>
>> Message:
>> Please take a look.
>>
>> Description:
>> Add user infor to switch
>>
>> Append current user's username to current environment
>> info printed out by `$ juju switch`.
>>
>>
>> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>>
>> (do not edit description out of merge proposal)
>>
>>
>> Please review this at https://codereview.appspot.com/92610043/
>>
>> Affected files (+91, -5 lines):
>>     A [revision details]
>>     M cmd/juju/switch.go
>>     M cmd/juju/switch_test.go
>>
>>
>> Index: [revision details]
>> === added file '[revision details]'
>> --- [revision details]  2012-01-01 00:00:00 +0000
>> +++ [revision details]  2012-01-01 00:00:00 +0000
>> @@ -0,0 +1,2 @@
>> +Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
>> +New revision: jesse.meek@canonical.com-20140525010706-80ir2jmt3rww66a0
>>
>> Index: cmd/juju/switch.go
>> === modified file 'cmd/juju/switch.go'
>> --- cmd/juju/switch.go  2014-05-09 13:24:50 +0000
>> +++ cmd/juju/switch.go  2014-05-25 01:07:06 +0000
>> @@ -4,16 +4,17 @@
>>    package main
>>
>>    import (
>> -       "errors"
>>          "fmt"
>>          "os"
>>          "sort"
>>
>> +       "github.com/juju/errors"
>>          "launchpad.net/gnuflag"
>>
>>          "launchpad.net/juju-core/cmd"
>>          "launchpad.net/juju-core/cmd/envcmd"
>>          "launchpad.net/juju-core/environs"
>> +       "launchpad.net/juju-core/environs/configstore"
>>    )
>>
>>    type SwitchCommand struct {
>> @@ -62,6 +63,26 @@
>>          return false
>>    }
>>
>> +// printEnvInfo appends the current user to the current environment and
>> prints
>> +// both to Stdout when `$ juju switch` is called
>> +func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
>> error) {
>> +       userInfo := ""
>> +       store, err := configstore.Default()
>> +       if err != nil {
>> +               return err
>> +       }
>> +       info, err := store.ReadInfo(envName)
>> +       if err != nil {
>> +               if !errors.IsNotFound(err) {
>> +                       return err
>> +               }
>> +       } else {
>> +               userInfo = fmt.Sprintf("user: %s\n",
>> info.APICredentials().User)
>> +       }
>> +       fmt.Fprint(ctx.Stdout, msg+userInfo)
>> +       return nil
>> +}
>> +
>>    func (c *SwitchCommand) Run(ctx *cmd.Context) error {
>>          // Switch is an alternative way of dealing with environments than
>> using
>>          // the JUJU_ENV environment setting, and as such, doesn't play too
>> well.
>> @@ -90,7 +111,10 @@
>>          jujuEnv := os.Getenv("JUJU_ENV")
>>          if jujuEnv != "" {
>>                  if c.EnvName == "" {
>> -                       fmt.Fprintf(ctx.Stdout, "%s\n", jujuEnv)
>> +                       msg := fmt.Sprintf("%s\n", jujuEnv)
>> +                       if err := printEnvInfo(ctx, msg, jujuEnv); err !=
>> nil {
>> +                               return err
>> +                       }
>>                          return nil
>>                  } else {
>>                          return fmt.Errorf("cannot switch when JUJU_ENV is
>> overriding the
>> environment (set to %q)", jujuEnv)
>> @@ -109,7 +133,10 @@
>>                  return errors.New("no currently specified environment")
>>          case c.EnvName == "":
>>                  // Simply print the current environment.
>> -               fmt.Fprintf(ctx.Stdout, "%s\n", currentEnv)
>> +               msg := fmt.Sprintf("%s\n", currentEnv)
>> +               if err := printEnvInfo(ctx, msg, currentEnv); err != nil {
>> +                       return err
>> +               }
>>          default:
>>                  // Switch the environment.
>>                  if !validEnvironmentName(c.EnvName, names) {
>> @@ -119,9 +146,15 @@
>>                          return err
>>                  }
>>                  if currentEnv == "" {
>> -                       fmt.Fprintf(ctx.Stdout, "-> %s\n", c.EnvName)
>> +                       msg := fmt.Sprintf("-> %s\n", c.EnvName)
>> +                       if err := printEnvInfo(ctx, msg, c.EnvName); err
>> != nil {
>> +                               return err
>> +                       }
>>                  } else {
>> -                       fmt.Fprintf(ctx.Stdout, "%s -> %s\n", currentEnv,
>> c.EnvName)
>> +                       msg := fmt.Sprintf("%s -> %s\n", currentEnv,
>> c.EnvName)
>> +                       if err := printEnvInfo(ctx, msg, c.EnvName); err
>> != nil {
>> +                               return err
>> +                       }
>>                  }
>>          }
>>          return nil
>>
>>
>> Index: cmd/juju/switch_test.go
>> === modified file 'cmd/juju/switch_test.go'
>> --- cmd/juju/switch_test.go     2014-05-22 06:03:06 +0000
>> +++ cmd/juju/switch_test.go     2014-05-25 01:07:06 +0000
>> @@ -9,6 +9,7 @@
>>          gc "launchpad.net/gocheck"
>>
>>          "launchpad.net/juju-core/cmd/envcmd"
>> +       "launchpad.net/juju-core/environs/configstore"
>>          _ "launchpad.net/juju-core/juju"
>>          "launchpad.net/juju-core/testing"
>>    )
>> @@ -19,6 +20,23 @@
>>
>>    var _ = gc.Suite(&SwitchSimpleSuite{})
>>
>> +var testCreds = configstore.APICredentials{
>> +       User:     "joblow",
>> +       Password: "baloney",
>> +}
>> +
>> +func patchEnvWithUser(c *gc.C, envName string) {
>> +       testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>> +
>> +       // Patch APICredentials so that <envName>.jenv file is avaliable
>> for
>> +       // switch to read the envirionment user from.
>> +       store, err := configstore.Default()
>> +       c.Assert(err, gc.IsNil)
>> +       info, err := store.CreateInfo(envName)
>> +       info.SetAPICredentials(testCreds)
>> +       info.Write()
>> +}
>> +
>>    func (*SwitchSimpleSuite) TestNoEnvironment(c *gc.C) {
>>          envPath := testing.HomePath(".juju", "environments.yaml")
>>          err := os.Remove(envPath)
>> @@ -40,6 +58,13 @@
>>          c.Assert(testing.Stdout(context), gc.Equals, "erewhemos\n")
>>    }
>>
>> +func (s *SwitchSimpleSuite) TestShowsDefaultAndUser(c *gc.C) {
>> +       patchEnvWithUser(c, "erewhemos")
>> +       context, err := testing.RunCommand(c, &SwitchCommand{})
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context),
>> gc.Equals, "erewhemos\nuser: "+testCreds.User+"\n")
>> +}
>> +
>>    func (s *SwitchSimpleSuite) TestCurrentEnvironmentHasPrecidence(c *gc.C)
>> {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          s.FakeHomeSuite.Home.AddFiles(c,
>> testing.TestFile{".juju/current-environment", "fubar"})
>> @@ -48,6 +73,14 @@
>>          c.Assert(testing.Stdout(context), gc.Equals, "fubar\n")
>>    }
>>
>> +func (s *SwitchSimpleSuite)
>> TestCurrentEnvironmentHasPrecidenceAndShowUser(c *gc.C) {
>> +       patchEnvWithUser(c, "fubar")
>> +       s.FakeHomeSuite.Home.AddFiles(c,
>> testing.TestFile{".juju/current-environment", "fubar"})
>> +       context, err := testing.RunCommand(c, &SwitchCommand{})
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context),
>> gc.Equals, "fubar\nuser: "+testCreds.User+"\n")
>> +}
>> +
>>    func (*SwitchSimpleSuite) TestShowsJujuEnv(c *gc.C) {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          os.Setenv("JUJU_ENV", "using-env")
>> @@ -56,6 +89,16 @@
>>          c.Assert(testing.Stdout(context), gc.Equals, "using-env\n")
>>    }
>>
>> +func (*SwitchSimpleSuite) TestShowsJujuEnvAndUser(c *gc.C) {
>> +       // testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>> +       envName := "using-env"
>> +       os.Setenv("JUJU_ENV", envName)
>> +       patchEnvWithUser(c, envName)
>> +       context, err := testing.RunCommand(c, &SwitchCommand{})
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context),
>> gc.Equals, "using-env\nuser: "+testCreds.User+"\n")
>> +}
>> +
>>    func (s *SwitchSimpleSuite) TestJujuEnvOverCurrentEnvironment(c *gc.C) {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          s.FakeHomeSuite.Home.AddFiles(c,
>> testing.TestFile{".juju/current-environment", "fubar"})
>> @@ -73,6 +116,14 @@
>>          c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
>>    }
>>
>> +func (*SwitchSimpleSuite) TestSettingWritesFileAndShowUser(c *gc.C) {
>> +       patchEnvWithUser(c, "erewhemos-2")
>> +       context, err := testing.RunCommand(c, &SwitchCommand{},
>> "erewhemos-2")
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context), gc.Equals, "erewhemos ->
>> erewhemos-2\nuser: "+testCreds.User+"\n")
>> +       c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
>> +}
>> +
>>    func (*SwitchSimpleSuite) TestSettingToUnknown(c *gc.C) {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          _, err := testing.RunCommand(c, &SwitchCommand{}, "unknown")
>>
>>
>>
>>
>>
>> --
>>
>> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>> Your team juju hackers is requested to review the proposed merge of
>> lp:~waigani/juju-core/switch-show-user into lp:juju-core.
>>

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-25:

#

Download full text (9.5 KiB)

oops, ignore the phone question. I thought I was replying to Rog
directly, facepalm.

On 26/05/14 10:21, Roger Peppe wrote:
> Implementation looks fine in general (+1 to Menno's remarks) but...
>
> Having juju switch print this seems a bit odd as it doesn't have anything
> else to do with users, does it? Is there a "juju user" command? If not we
> could make one. BTW this change may potentially break backwards
> compatibility with sh scripts I think.
> On 25 May 2014 02:15, "Jesse Meek" <email address hidden> wrote:
>
>> Reviewers: mp+220868_code.launchpad.net,
>>
>> Message:
>> Please take a look.
>>
>> Description:
>> Add user infor to switch
>>
>> Append current user's username to current environment
>> info printed out by `$ juju switch`.
>>
>>
>> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>>
>> (do not edit description out of merge proposal)
>>
>>
>> Please review this at https://codereview.appspot.com/92610043/
>>
>> Affected files (+91, -5 lines):
>> A [revision details]
>> M cmd/juju/switch.go
>> M cmd/juju/switch_test.go
>>
>>
>> Index: [revision details]
>> === added file '[revision details]'
>> --- [revision details] 2012-01-01 00:00:00 +0000
>> +++ [revision details] 2012-01-01 00:00:00 +0000
>> @@ -0,0 +1,2 @@
>> +Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
>> +New revision: <email address hidden>
>>
>> Index: cmd/juju/switch.go
>> === modified file 'cmd/juju/switch.go'
>> --- cmd/juju/switch.go 2014-05-09 13:24:50 +0000
>> +++ cmd/juju/switch.go 2014-05-25 01:07:06 +0000
>> @@ -4,16 +4,17 @@
>> package main
>>
>> import (
>> - "errors"
>> "fmt"
>> "os"
>> "sort"
>>
>> + "github.com/juju/errors"
>> "launchpad.net/gnuflag"
>>
>> "launchpad.net/juju-core/cmd"
>> "launchpad.net/juju-core/cmd/envcmd"
>> "launchpad.net/juju-core/environs"
>> + "launchpad.net/juju-core/environs/configstore"
>> )
>>
>> type SwitchCommand struct {
>> @@ -62,6 +63,26 @@
>> return false
>> }
>>
>> +// printEnvInfo appends the current user to the current environment and
>> prints
>> +// both to Stdout when `$ juju switch` is called
>> +func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
>> error) {
>> + userInfo := ""
>> + store, err := configstore.Default()
>> + if err != nil {
>> + return err
>> + }
>> + info, err := store.ReadInfo(envName)
>> + if err != nil {
>> + if !errors.IsNotFound(err) {
>> + return err
>> + }
>> + } else {
>> + userInfo = fmt.Sprintf("user: %s\n",
>> info.APICredentials().User)
>> + }
>> + fmt.Fprint(ctx.Stdout, msg+userInfo)
>> + return nil
>> +}
>> +
>> func (c *SwitchCommand) Run(ctx *cmd.Context) error {
>> // Switch is an alternative way of dealing with environments than
>> using
>> // the JUJU_ENV environment setting, and as such, doesn't play too
>> well.
>> @@ -90,7 +111,10 @@
>> jujuEnv := os.Getenv("JUJU_ENV")
>> if jujuEnv != "" {...

oops, ignore the phone question. I thought I was replying to Rog 
directly, facepalm.

On 26/05/14 10:21, Roger Peppe wrote:
> Implementation looks fine in general (+1 to Menno's remarks) but...
>
> Having juju switch print this seems a bit odd as it doesn't have anything
> else to do with users, does it? Is there a "juju user" command? If not we
> could make one. BTW this change may potentially break backwards
> compatibility with sh scripts I think.
> On 25 May 2014 02:15, "Jesse Meek" <jesse.meek@canonical.com> wrote:
>
>> Reviewers: mp+220868_code.launchpad.net,
>>
>> Message:
>> Please take a look.
>>
>> Description:
>> Add user infor to switch
>>
>> Append current user's username to current environment
>> info printed out by `$ juju switch`.
>>
>>
>> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>>
>> (do not edit description out of merge proposal)
>>
>>
>> Please review this at https://codereview.appspot.com/92610043/
>>
>> Affected files (+91, -5 lines):
>>     A [revision details]
>>     M cmd/juju/switch.go
>>     M cmd/juju/switch_test.go
>>
>>
>> Index: [revision details]
>> === added file '[revision details]'
>> --- [revision details]  2012-01-01 00:00:00 +0000
>> +++ [revision details]  2012-01-01 00:00:00 +0000
>> @@ -0,0 +1,2 @@
>> +Old revision: tarmac-20140523013529-f0gwneum0rsp1vfe
>> +New revision: jesse.meek@canonical.com-20140525010706-80ir2jmt3rww66a0
>>
>> Index: cmd/juju/switch.go
>> === modified file 'cmd/juju/switch.go'
>> --- cmd/juju/switch.go  2014-05-09 13:24:50 +0000
>> +++ cmd/juju/switch.go  2014-05-25 01:07:06 +0000
>> @@ -4,16 +4,17 @@
>>    package main
>>
>>    import (
>> -       "errors"
>>          "fmt"
>>          "os"
>>          "sort"
>>
>> +       "github.com/juju/errors"
>>          "launchpad.net/gnuflag"
>>
>>          "launchpad.net/juju-core/cmd"
>>          "launchpad.net/juju-core/cmd/envcmd"
>>          "launchpad.net/juju-core/environs"
>> +       "launchpad.net/juju-core/environs/configstore"
>>    )
>>
>>    type SwitchCommand struct {
>> @@ -62,6 +63,26 @@
>>          return false
>>    }
>>
>> +// printEnvInfo appends the current user to the current environment and
>> prints
>> +// both to Stdout when `$ juju switch` is called
>> +func printEnvInfo(ctx *cmd.Context, msg string, envName string) (err
>> error) {
>> +       userInfo := ""
>> +       store, err := configstore.Default()
>> +       if err != nil {
>> +               return err
>> +       }
>> +       info, err := store.ReadInfo(envName)
>> +       if err != nil {
>> +               if !errors.IsNotFound(err) {
>> +                       return err
>> +               }
>> +       } else {
>> +               userInfo = fmt.Sprintf("user: %s\n",
>> info.APICredentials().User)
>> +       }
>> +       fmt.Fprint(ctx.Stdout, msg+userInfo)
>> +       return nil
>> +}
>> +
>>    func (c *SwitchCommand) Run(ctx *cmd.Context) error {
>>          // Switch is an alternative way of dealing with environments than
>> using
>>          // the JUJU_ENV environment setting, and as such, doesn't play too
>> well.
>> @@ -90,7 +111,10 @@
>>          jujuEnv := os.Getenv("JUJU_ENV")
>>          if jujuEnv != "" {
>>                  if c.EnvName == "" {
>> -                       fmt.Fprintf(ctx.Stdout, "%s\n", jujuEnv)
>> +                       msg := fmt.Sprintf("%s\n", jujuEnv)
>> +                       if err := printEnvInfo(ctx, msg, jujuEnv); err !=
>> nil {
>> +                               return err
>> +                       }
>>                          return nil
>>                  } else {
>>                          return fmt.Errorf("cannot switch when JUJU_ENV is
>> overriding the
>> environment (set to %q)", jujuEnv)
>> @@ -109,7 +133,10 @@
>>                  return errors.New("no currently specified environment")
>>          case c.EnvName == "":
>>                  // Simply print the current environment.
>> -               fmt.Fprintf(ctx.Stdout, "%s\n", currentEnv)
>> +               msg := fmt.Sprintf("%s\n", currentEnv)
>> +               if err := printEnvInfo(ctx, msg, currentEnv); err != nil {
>> +                       return err
>> +               }
>>          default:
>>                  // Switch the environment.
>>                  if !validEnvironmentName(c.EnvName, names) {
>> @@ -119,9 +146,15 @@
>>                          return err
>>                  }
>>                  if currentEnv == "" {
>> -                       fmt.Fprintf(ctx.Stdout, "-> %s\n", c.EnvName)
>> +                       msg := fmt.Sprintf("-> %s\n", c.EnvName)
>> +                       if err := printEnvInfo(ctx, msg, c.EnvName); err
>> != nil {
>> +                               return err
>> +                       }
>>                  } else {
>> -                       fmt.Fprintf(ctx.Stdout, "%s -> %s\n", currentEnv,
>> c.EnvName)
>> +                       msg := fmt.Sprintf("%s -> %s\n", currentEnv,
>> c.EnvName)
>> +                       if err := printEnvInfo(ctx, msg, c.EnvName); err
>> != nil {
>> +                               return err
>> +                       }
>>                  }
>>          }
>>          return nil
>>
>>
>> Index: cmd/juju/switch_test.go
>> === modified file 'cmd/juju/switch_test.go'
>> --- cmd/juju/switch_test.go     2014-05-22 06:03:06 +0000
>> +++ cmd/juju/switch_test.go     2014-05-25 01:07:06 +0000
>> @@ -9,6 +9,7 @@
>>          gc "launchpad.net/gocheck"
>>
>>          "launchpad.net/juju-core/cmd/envcmd"
>> +       "launchpad.net/juju-core/environs/configstore"
>>          _ "launchpad.net/juju-core/juju"
>>          "launchpad.net/juju-core/testing"
>>    )
>> @@ -19,6 +20,23 @@
>>
>>    var _ = gc.Suite(&SwitchSimpleSuite{})
>>
>> +var testCreds = configstore.APICredentials{
>> +       User:     "joblow",
>> +       Password: "baloney",
>> +}
>> +
>> +func patchEnvWithUser(c *gc.C, envName string) {
>> +       testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>> +
>> +       // Patch APICredentials so that <envName>.jenv file is avaliable
>> for
>> +       // switch to read the envirionment user from.
>> +       store, err := configstore.Default()
>> +       c.Assert(err, gc.IsNil)
>> +       info, err := store.CreateInfo(envName)
>> +       info.SetAPICredentials(testCreds)
>> +       info.Write()
>> +}
>> +
>>    func (*SwitchSimpleSuite) TestNoEnvironment(c *gc.C) {
>>          envPath := testing.HomePath(".juju", "environments.yaml")
>>          err := os.Remove(envPath)
>> @@ -40,6 +58,13 @@
>>          c.Assert(testing.Stdout(context), gc.Equals, "erewhemos\n")
>>    }
>>
>> +func (s *SwitchSimpleSuite) TestShowsDefaultAndUser(c *gc.C) {
>> +       patchEnvWithUser(c, "erewhemos")
>> +       context, err := testing.RunCommand(c, &SwitchCommand{})
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context),
>> gc.Equals, "erewhemos\nuser: "+testCreds.User+"\n")
>> +}
>> +
>>    func (s *SwitchSimpleSuite) TestCurrentEnvironmentHasPrecidence(c *gc.C)
>> {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          s.FakeHomeSuite.Home.AddFiles(c,
>> testing.TestFile{".juju/current-environment", "fubar"})
>> @@ -48,6 +73,14 @@
>>          c.Assert(testing.Stdout(context), gc.Equals, "fubar\n")
>>    }
>>
>> +func (s *SwitchSimpleSuite)
>> TestCurrentEnvironmentHasPrecidenceAndShowUser(c *gc.C) {
>> +       patchEnvWithUser(c, "fubar")
>> +       s.FakeHomeSuite.Home.AddFiles(c,
>> testing.TestFile{".juju/current-environment", "fubar"})
>> +       context, err := testing.RunCommand(c, &SwitchCommand{})
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context),
>> gc.Equals, "fubar\nuser: "+testCreds.User+"\n")
>> +}
>> +
>>    func (*SwitchSimpleSuite) TestShowsJujuEnv(c *gc.C) {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          os.Setenv("JUJU_ENV", "using-env")
>> @@ -56,6 +89,16 @@
>>          c.Assert(testing.Stdout(context), gc.Equals, "using-env\n")
>>    }
>>
>> +func (*SwitchSimpleSuite) TestShowsJujuEnvAndUser(c *gc.C) {
>> +       // testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>> +       envName := "using-env"
>> +       os.Setenv("JUJU_ENV", envName)
>> +       patchEnvWithUser(c, envName)
>> +       context, err := testing.RunCommand(c, &SwitchCommand{})
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context),
>> gc.Equals, "using-env\nuser: "+testCreds.User+"\n")
>> +}
>> +
>>    func (s *SwitchSimpleSuite) TestJujuEnvOverCurrentEnvironment(c *gc.C) {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          s.FakeHomeSuite.Home.AddFiles(c,
>> testing.TestFile{".juju/current-environment", "fubar"})
>> @@ -73,6 +116,14 @@
>>          c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
>>    }
>>
>> +func (*SwitchSimpleSuite) TestSettingWritesFileAndShowUser(c *gc.C) {
>> +       patchEnvWithUser(c, "erewhemos-2")
>> +       context, err := testing.RunCommand(c, &SwitchCommand{},
>> "erewhemos-2")
>> +       c.Assert(err, gc.IsNil)
>> +       c.Assert(testing.Stdout(context), gc.Equals, "erewhemos ->
>> erewhemos-2\nuser: "+testCreds.User+"\n")
>> +       c.Assert(envcmd.ReadCurrentEnvironment(), gc.Equals, "erewhemos-2")
>> +}
>> +
>>    func (*SwitchSimpleSuite) TestSettingToUnknown(c *gc.C) {
>>          testing.WriteEnvironments(c, testing.MultipleEnvConfig)
>>          _, err := testing.RunCommand(c, &SwitchCommand{}, "unknown")
>>
>>
>>
>>
>>
>> --
>>
>> https://code.launchpad.net/~waigani/juju-core/switch-show-user/+merge/220868
>> Your team juju hackers is requested to review the proposed merge of
>> lp:~waigani/juju-core/switch-show-user into lp:juju-core.
>>

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-26:

#

Please take a look.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Tim Penhey (thumper) wrote on 2014-05-26:

#

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

https://codereview.appspot.com/92610043/diff/20001/cmd/juju/switch_test.go#newcode24
cmd/juju/switch_test.go:24: User: "joblow",
Can we just use "joe" here? For reasons explained on IRC.

https://codereview.appspot.com/92610043/diff/20001/cmd/juju/switch_test.go#newcode123
cmd/juju/switch_test.go:123: c.Assert(testing.Stdout(context),
gc.Equals, "erewhemos -> erewhemos-2\nuser: "+testCreds.User+"\n")
I think this would be better as:

erewhemos -> erewhemos-2 as user "joe"

Also, best to make the expected text explicit if you can.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-26:

#

Please take a look.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Tim Penhey (thumper) wrote on 2014-05-26:

#

https://codereview.appspot.com/92610043/diff/40001/cmd/juju/switch_test.go
File cmd/juju/switch_test.go (right):

https://codereview.appspot.com/92610043/diff/40001/cmd/juju/switch_test.go#newcode65
cmd/juju/switch_test.go:65: c.Assert(testing.Stdout(context), gc.Equals,
"erewhemos as user "+testCreds.User+"\n")
As I mentioned before, make the string explicit:

c.Assert(testing.Stdout(context), gc.Equals, `erewhemos as user "joe"` +
"\n")

Horrible having to add the + "\n" to the end, but better than
...\"joe\"\n"

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-26:

#

On 2014/05/26 01:46:37, thumper wrote:

https://codereview.appspot.com/92610043/diff/40001/cmd/juju/switch_test.go
> File cmd/juju/switch_test.go (right):

https://codereview.appspot.com/92610043/diff/40001/cmd/juju/switch_test.go#newcode65
> cmd/juju/switch_test.go:65: c.Assert(testing.Stdout(context),
gc.Equals,
> "erewhemos as user "+testCreds.User+"\n")
> As I mentioned before, make the string explicit:

> c.Assert(testing.Stdout(context), gc.Equals, `erewhemos as user "joe"`
+ "\n")

> Horrible having to add the + "\n" to the end, but better than
...\"joe\"\n"

oops, missed that. Done now.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-26:

#

Please take a look.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Tim Penhey (thumper) wrote on 2014-05-26:

#

https://codereview.appspot.com/92610043/diff/60001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/60001/cmd/juju/switch.go#newcode149
cmd/juju/switch.go:149: if username != "" {
Hmm... is this implied "admin"?

https://codereview.appspot.com/92610043/diff/60001/cmd/juju/switch_test.go
File cmd/juju/switch_test.go (right):

https://codereview.appspot.com/92610043/diff/60001/cmd/juju/switch_test.go#newcode65
cmd/juju/switch_test.go:65: c.Assert(testing.Stdout(context), gc.Equals,
`erewhemos as user "`+testCreds.User+"\"\n")
umm... your earlier comment said you fixed this. Did you commit it?

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-26:

#

Please take a look.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-26:

#

https://codereview.appspot.com/92610043/diff/60001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/60001/cmd/juju/switch.go#newcode149
cmd/juju/switch.go:149: if username != "" {
On 2014/05/26 04:09:42, thumper wrote:
> Hmm... is this implied "admin"?

If there is no .jenv file, username is not appended to output of switch.
Are you saying that instead the "admin" user should be appended?

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

William Reade (fwereade) wrote on 2014-05-26:

#

I'm not convinced by this. It's a compatibility break (which is not in
itself necessarily bad -- we should be better when we can be) but (1)
AIUI, people actively use this in scripts and (2) I'm not convinced it
actually offers anything significantly better.

* username is quoted but envname isn't
* username sometimes shows up and sometimes doesn't
* it's going to be kinda annoying to parse

ISTM that if we're going to Do It Right we should collect a map (or even
a struct) of all the relevant information and give it a --format arg (in
which the default "smart" (lol) formatting can match the existing output
for the next release. Counterarguments?

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

William Reade (fwereade) wrote on 2014-05-26:

#

On 2014/05/26 07:26:56, fwereade wrote:
> I'm not convinced by this. It's a compatibility break (which is not in
itself
> necessarily bad -- we should be better when we can be) but (1) AIUI,
people
> actively use this in scripts and (2) I'm not convinced it actually
offers
> anything significantly better.

> * username is quoted but envname isn't
> * username sometimes shows up and sometimes doesn't
> * it's going to be kinda annoying to parse

> ISTM that if we're going to Do It Right we should collect a map (or
even a
> struct) of all the relevant information and give it a --format arg (in
which the
> default "smart" (lol) formatting can match the existing output for the
next
> release. Counterarguments?

*also*, this is *unquestionably* user-facing. We need docs for such
features to exist at the time of writing, so that evilnick can see them
and have a chance to massage them into a happy shape before the feature
ends up in front of our users.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-27:

#

Please take a look.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

William Reade (fwereade) wrote on 2014-05-27:

#

Coming along nicely, thanks.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode53
cmd/juju/switch.go:53: $ juju switch --env-info
Not *really* loving --env-info... "--explain" isn't great either, but it
feels a bit closer to what we're asking for... or what about "--long"?
we want to make it super-easy for people to use this, and get used to it
-- for that matter, we should probably have a single-char flag too)

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode55
cmd/juju/switch.go:55: apiEndpoint:
I think we need:

environ-name [0]
environ-uuid
state-servers
user-name

...but I'm not so sure ca-cert is very nice to include, because it'll be
MASSIVE and push interesting stuff offscreen.

[0] environ-name is strange and interesting, fwiw -- I think it can only
ever really *sanely* be a local alias for a user/env pair -- but we *do*
have environment names lurking in the state, meaningless though they may
be. Can you think of a better way to describe what it actually is?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode60
cmd/juju/switch.go:60: envName: local
please use "names-with-hyphens", not namesWithCaps, for consistency.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode72
cmd/juju/switch.go:72: oldEnvName: ec2
previous-environ-name perhaps?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode77
cmd/juju/switch.go:77: {"apiEndpoint":
similarly, marshal to json with nice-names-like-this.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode83
cmd/juju/switch.go:83: "username":"joe"
Thanks for the docs (for all that they're still in a state of flux) -- I
find it *much* easier to think through the consistency and consequences
and so on when the UX is laid out plainly like this.

Please make sure evilnick knows about them so he has a chance to fix up
the online docs for the next release -- we absolutely want his feedback
on what's a good and easy-to-communicate experience.

https://codereview.appspot.com/92610043/

Coming along nicely, thanks.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode53
cmd/juju/switch.go:53: $ juju switch --env-info
Not *really* loving --env-info... "--explain" isn't great either, but it
feels a bit closer to what we're asking for... or what about "--long"?
we want to make it super-easy for people to use this, and get used to it
-- for that matter, we should probably have a single-char flag too)

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode55
cmd/juju/switch.go:55: apiEndpoint:
I think we need:

environ-name [0]
environ-uuid
state-servers
user-name

...but I'm not so sure ca-cert is very nice to include, because it'll be
MASSIVE and push interesting stuff offscreen.

[0] environ-name is strange and interesting, fwiw -- I think it can only
ever really *sanely* be a local alias for a user/env pair -- but we *do*
have environment names lurking in the state, meaningless though they may
be. Can you think of a better way to describe what it actually is?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode60
cmd/juju/switch.go:60: envName: local
please use "names-with-hyphens", not namesWithCaps, for consistency.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode72
cmd/juju/switch.go:72: oldEnvName: ec2
previous-environ-name perhaps?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode77
cmd/juju/switch.go:77: {"apiEndpoint":
similarly, marshal to json with nice-names-like-this.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode83
cmd/juju/switch.go:83: "username":"joe"
Thanks for the docs (for all that they're still in a state of flux) -- I
find it *much* easier to think through the consistency and consequences
and so on when the UX is laid out plainly like this.

Please make sure evilnick knows about them so he has a chance to fix up
the online docs for the next release -- we absolutely want his feedback
on what's a good and easy-to-communicate experience.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Menno Finlay-Smits (menno.smits) wrote on 2014-05-27:

#

Just some little things I noticed

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode76
cmd/juju/switch.go:76: # Format environment inforamtion as json
information

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode190
cmd/juju/switch.go:190: }
Can't the whole "if currentEnv" block now be replaced with:

if err := c.printEnvInfo(ctx, c.EnvName, currentEnv); err != nil {
return err
}

?

(The first case passes "" when currentEnv is "")

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode251
cmd/juju/switch.go:251: return username, nil
Because this function uses named return values this line can just be
"return" if you like.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Jesse Meek (waigani) wrote on 2014-05-28:

#

Download full text (3.8 KiB)

Please take a look.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode53
cmd/juju/switch.go:53: $ juju switch --env-info
On 2014/05/27 07:47:12, fwereade wrote:
> Not *really* loving --env-info... "--explain" isn't great either, but
it feels a
> bit closer to what we're asking for... or what about "--long"? we want
to make
> it super-easy for people to use this, and get used to it -- for that
matter, we
> should probably have a single-char flag too)

We already have single-char flag (-e), added example to docs. If we use
"--long" we'd conflict with single-char flag for "--list", -li and/or
-lo?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode55
cmd/juju/switch.go:55: apiEndpoint:
On 2014/05/27 07:47:12, fwereade wrote:
> I think we need:

> environ-name [0]
> environ-uuid
> state-servers
> user-name

> ...but I'm not so sure ca-cert is very nice to include, because it'll
be MASSIVE
> and push interesting stuff offscreen.

> [0] environ-name is strange and interesting, fwiw -- I think it can
only ever
> really *sanely* be a local alias for a user/env pair -- but we *do*
have
> environment names lurking in the state, meaningless though they may
be. Can you
> think of a better way to describe what it actually is?

My understanding of environ-name is that it is simply the name of an
environment, defined in environments.yaml. An environ must always have a
user, but I don't see how that makes environ-name anything but the name
of an environment still? Am I missing something? Can you give me an
example of the problem?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode60
cmd/juju/switch.go:60: envName: local
On 2014/05/27 07:47:12, fwereade wrote:
> please use "names-with-hyphens", not namesWithCaps, for consistency.

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode72
cmd/juju/switch.go:72: oldEnvName: ec2
On 2014/05/27 07:47:13, fwereade wrote:
> previous-environ-name perhaps?

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode76
cmd/juju/switch.go:76: # Format environment inforamtion as json
On 2014/05/27 21:16:05, menn0 wrote:
> information

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode77
cmd/juju/switch.go:77: {"apiEndpoint":
On 2014/05/27 07:47:12, fwereade wrote:
> similarly, marshal to json with nice-names-like-this.

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode83
cmd/juju/switch.go:83: "username":"joe"
On 2014/05/27 07:47:13, fwereade wrote:
> Thanks for the docs (for all that they're still in a state of flux) --
I find it
> *much* easier to think through the consistency and consequences and so
on when
> the UX is laid out plainly like this.

> Please make sure evilnick knows about them so he has a chance to fix
up the
> online docs for the next release -- we absolutely want his feedback on
what's a
> good and easy-to-communicate experience.

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/...

Please take a look.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode53
cmd/juju/switch.go:53: $ juju switch --env-info
On 2014/05/27 07:47:12, fwereade wrote:
> Not *really* loving --env-info... "--explain" isn't great either, but
it feels a
> bit closer to what we're asking for... or what about "--long"? we want
to make
> it super-easy for people to use this, and get used to it -- for that
matter, we
> should probably have a single-char flag too)

We already have single-char flag (-e), added example to docs. If we use
"--long" we'd conflict with single-char flag for "--list", -li and/or
-lo?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode55
cmd/juju/switch.go:55: apiEndpoint:
On 2014/05/27 07:47:12, fwereade wrote:
> I think we need:

> environ-name [0]
> environ-uuid
> state-servers
> user-name

> ...but I'm not so sure ca-cert is very nice to include, because it'll
be MASSIVE
> and push interesting stuff offscreen.

> [0] environ-name is strange and interesting, fwiw -- I think it can
only ever
> really *sanely* be a local alias for a user/env pair -- but we *do*
have
> environment names lurking in the state, meaningless though they may
be. Can you
> think of a better way to describe what it actually is?

My understanding of environ-name is that it is simply the name of an
environment, defined in environments.yaml. An environ must always have a
user, but I don't see how that makes environ-name anything but the name
of an environment still? Am I missing something? Can you give me an
example of the problem?

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode60
cmd/juju/switch.go:60: envName: local
On 2014/05/27 07:47:12, fwereade wrote:
> please use "names-with-hyphens", not namesWithCaps, for consistency.

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode72
cmd/juju/switch.go:72: oldEnvName: ec2
On 2014/05/27 07:47:13, fwereade wrote:
> previous-environ-name perhaps?

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode76
cmd/juju/switch.go:76: # Format environment inforamtion as json
On 2014/05/27 21:16:05, menn0 wrote:
> information

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode77
cmd/juju/switch.go:77: {"apiEndpoint":
On 2014/05/27 07:47:12, fwereade wrote:
> similarly, marshal to json with nice-names-like-this.

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode83
cmd/juju/switch.go:83: "username":"joe"
On 2014/05/27 07:47:13, fwereade wrote:
> Thanks for the docs (for all that they're still in a state of flux) --
I find it
> *much* easier to think through the consistency and consequences and so
on when
> the UX is laid out plainly like this.

> Please make sure evilnick knows about them so he has a chance to fix
up the
> online docs for the next release -- we absolutely want his feedback on
what's a
> good and easy-to-communicate experience.

Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode190
cmd/juju/switch.go:190: }
On 2014/05/27 21:16:05, menn0 wrote:
> Can't the whole "if currentEnv" block now be replaced with:

> if err := c.printEnvInfo(ctx, c.EnvName, currentEnv); err != nil {
>       return err
> }

> ?

> (The first case passes "" when currentEnv is "")

Nice spotting! Done.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode251
cmd/juju/switch.go:251: return username, nil
On 2014/05/27 21:16:04, menn0 wrote:
> Because this function uses named return values this line can just be
"return" if
> you like.

This would return an 'is not found' error, when we don't want to.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

Tim Penhey (thumper) wrote on 2014-05-28:

#

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go#newcode53
cmd/juju/switch.go:53: $ juju switch --env-info
Why is this not just:
juju switch --format=yaml
?

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go#newcode70
cmd/juju/switch.go:70: $ juju switch local --env-info
Same here:
juju switch local --format=yaml

I don't think we need --env-info at all.

https://codereview.appspot.com/92610043/

Reply

Revision history for this message

William Reade (fwereade) wrote on 2014-05-28:

#

Download full text (3.7 KiB)

quick comments while I can, I'll try to do some more before you get
online

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode55
cmd/juju/switch.go:55: apiEndpoint:
On 2014/05/28 00:30:47, waigani wrote:
> On 2014/05/27 07:47:12, fwereade wrote:
> > I think we need:
> >
> > environ-name [0]
> > environ-uuid
> > state-servers
> > user-name
> >
> > ...but I'm not so sure ca-cert is very nice to include, because
it'll be
> MASSIVE
> > and push interesting stuff offscreen.
> >
> > [0] environ-name is strange and interesting, fwiw -- I think it can
only ever
> > really *sanely* be a local alias for a user/env pair -- but we *do*
have
> > environment names lurking in the state, meaningless though they may
be. Can
> you
> > think of a better way to describe what it actually is?

> My understanding of environ-name is that it is simply the name of an
> environment, defined in environments.yaml. An environ must always have
a user,
> but I don't see how that makes environ-name anything but the name of
an
> environment still? Am I missing something? Can you give me an example
of the
> problem?

I have an environment named "prod", and so do you. I give you access to
mine. How do you disambiguate? Environment name was a really bad idea --
we made it something fundamental, instead of explicitly making it a
local alias, and we continue to pay the price; but we're making some
progress, because what you're actually choosing in the general case is
now a *jenv* name, not an environment name.

https://codereview.appspot.com/92610043/diff/90001/cmd/juju/switch.go#newcode251
cmd/juju/switch.go:251: return username, nil
On 2014/05/28 00:30:47, waigani wrote:
> On 2014/05/27 21:16:04, menn0 wrote:
> > Because this function uses named return values this line can just be
"return"
> if
> > you like.

> This would return an 'is not found' error, when we don't want to.

And FWIW I have generally found that the benefits of empty returns are
outweighed by the costs of (frequently) having to scan back through the
function to be sure what actually happened.

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go
File cmd/juju/switch.go (right):

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go#newcode45
cmd/juju/switch.go:45: # Show current environment name
General suggestion:

# description
$ command
output
output
output

?

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go#newcode53
cmd/juju/switch.go:53: $ juju switch --env-info
On 2014/05/28 02:26:46, thumper wrote:
> Why is this not just:
> juju switch --format=yaml
> ?

Very good point. We can define a "smart" (lol) formatter that preserves
existing behaviour.

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go#newcode197
cmd/juju/switch.go:197: servers, err := stateSevers(envName)
s/Severs/Servers/

https://codereview.appspot.com/92610043/diff/110001/cmd/juju/switch.go#newcode205
cmd/juju/switch.go:205: "state-servers": servers,
I'd be more inclined to define this as a struct, with explicit
marshalling tags...