Merge pull request #333 from lets-cli/codex/make-upgrade-selfcommand-308

Replace root upgrade flag with self upgrade command

Author: kindermax

docs/docs/architecture.md

@@ -71,7 +71,8 @@ To achieve this we are creating so-called `root` command and `subcommands` from
 #### Root command
 
 Root command is responsible for:
-  - `lets` own command line flags such as `--version`, `--upgrade`, `--help` and so on.
+  - `lets` own command line flags such as `--version`, `--help` and so on.
+  - `lets self` subcommands such as `lets self upgrade`
   - `lets` commands autocompletion in terminal
 
 #### Subcommands

docs/docs/changelog.md

@@ -22,6 +22,7 @@ title: Changelog
 * `[Fixed]` Resolve `go to definition` from YAML merge aliases such as `<<: *test` to the referenced command in `lets self lsp`.
 * `[Fixed]` Resolve `go to definition` from command references such as `ref: build` to the referenced command in `lets self lsp`.
 * `[Added]` Load local mixin files into LSP storage and command index so mixin commands are available for navigation.
+* `[Changed]` Replace the top-level `--upgrade` flag with the `lets self upgrade` command.
 
 ## [0.0.59](https://github.com/lets-cli/lets/releases/tag/v0.0.59)
 

docs/docs/cli.md

@@ -12,9 +12,10 @@ title: CLI options
 |`--init`|`bool`|false|creates a new lets.yaml in the current folder|
 |`--only`|`stringArray`||run only specified command(s) described in cmd as map|
 |`--exclude`|`stringArray`||run all but excluded command(s) described in cmd as map|
-|`--upgrade`|`bool`|false|upgrade lets to latest version|
 |`--no-depends`|`bool`|false|skip 'depends' for running command|
 |`-c, --config`|`string`|lets.yaml|specify config|
 |`-d, --debug`|`bool`|false|verbose logs|
 |`-h, --help`|||help for lets|
 |`-v, --version`|||version for lets|
+
+Upgrade the lets binary with `lets self upgrade`.

docs/docs/installation.mdx

@@ -126,13 +126,13 @@ brew install lets-cli/tap/lets
 >
 <TabItem value="self">
 
-Starting from version `0.0.30` lets has `--upgrade` flag which allows to do self-upgrades.
+Starting from version `0.0.30` lets has a built-in self-upgrade command.
 
 It updates binary located at `which lets`
 
 
 ```bash
-lets --upgrade
+lets self upgrade
 ```
 
 If your `lets` version is below `0.0.30` - use shell script and specify the latest version.

internal/cli/cli.go

@@ -112,20 +112,6 @@ func Main(version string, buildDate string) int {
 		return 0
 	}
 
-	if rootFlags.upgrade {
-		upgrader, err := upgrade.NewBinaryUpgrader(registry.NewGithubRegistry(), version)
-		if err == nil {
-			err = upgrader.Upgrade(ctx)
-		}
-
-		if err != nil {
-			log.Errorf("can not self-upgrade binary: %s", err)
-			return 1
-		}
-
-		return 0
-	}
-
 	showUsage := rootFlags.help || (command.Name() == "help" && len(args) == 0) || (len(os.Args) == 1)
 
 	if showUsage {
@@ -204,6 +190,10 @@ func allowsMissingConfig(current *cobra.Command) bool {
 		return true
 	}
 
+	return isSelfCommand(current)
+}
+
+func isSelfCommand(current *cobra.Command) bool {
 	for cmd := current; cmd != nil; cmd = cmd.Parent() {
 		parent := cmd.Parent()
 		if cmd.Name() == "self" && parent != nil && parent.Name() == "lets" {
@@ -220,7 +210,7 @@ func maybeStartUpdateCheck(
 	command *cobra.Command,
 	appSettings settings.Settings,
 ) (<-chan updateCheckResult, context.CancelFunc) {
-	if !shouldCheckForUpdate(command.Name(), isInteractiveStderr(), appSettings) {
+	if !shouldCheckForUpdate(command, isInteractiveStderr(), appSettings) {
 		return nil, func() {}
 	}
 
@@ -273,17 +263,21 @@ func printUpdateNotice(updateCh <-chan updateCheckResult) {
 	}
 }
 
-func shouldCheckForUpdate(commandName string, interactive bool, appSettings settings.Settings) bool {
+func shouldCheckForUpdate(command *cobra.Command, interactive bool, appSettings settings.Settings) bool {
 	if !interactive || !appSettings.UpgradeNotify || os.Getenv("CI") != "" {
 		return false
 	}
 
-	switch commandName {
-	case "completion", "help", "lsp", "self":
-		return false
-	default:
+	if command == nil {
 		return true
 	}
+
+	switch command.Name() {
+	case "completion", "help":
+		return false
+	}
+
+	return !isSelfCommand(command)
 }
 
 func isInteractiveStderr() bool {
@@ -298,7 +292,6 @@ type flags struct {
 	version bool
 	all     bool
 	init    bool
-	upgrade bool
 }
 
 // We can not parse --config and --debug flags using cobra.Command.ParseFlags
@@ -379,9 +372,7 @@ func parseRootFlags(args []string) (*flags, error) {
 				f.init = true
 			}
 		case "--upgrade":
-			if !isFlagVisited("upgrade") {
-				f.upgrade = true
-			}
+			return nil, errors.New("--upgrade has been replaced with 'lets self upgrade'")
 		}
 
 		idx += 1 //nolint:revive,golint

internal/cli/cli_test.go

@@ -65,20 +65,20 @@ func TestShouldCheckForUpdate(t *testing.T) {
 	t.Run("should allow normal interactive commands", func(t *testing.T) {
 		t.Setenv("CI", "")
 
-		if !shouldCheckForUpdate("lets", true, defaultSettings) {
+		if !shouldCheckForUpdate(&cobra.Command{Use: "run"}, true, defaultSettings) {
 			t.Fatal("expected update check to be enabled")
 		}
 	})
 
 	t.Run("should skip non interactive sessions", func(t *testing.T) {
-		if shouldCheckForUpdate("lets", false, defaultSettings) {
+		if shouldCheckForUpdate(&cobra.Command{Use: "run"}, false, defaultSettings) {
 			t.Fatal("expected non-interactive session to skip update check")
 		}
 	})
 
 	t.Run("should skip when CI is set", func(t *testing.T) {
 		t.Setenv("CI", "1")
-		if shouldCheckForUpdate("lets", true, defaultSettings) {
+		if shouldCheckForUpdate(&cobra.Command{Use: "run"}, true, defaultSettings) {
 			t.Fatal("expected CI to skip update check")
 		}
 	})
@@ -87,16 +87,45 @@ func TestShouldCheckForUpdate(t *testing.T) {
 		disabled := settings.Default()
 		disabled.UpgradeNotify = false
 
-		if shouldCheckForUpdate("lets", true, disabled) {
+		if shouldCheckForUpdate(&cobra.Command{Use: "run"}, true, disabled) {
 			t.Fatal("expected disabled settings to skip update check")
 		}
 	})
 
-	t.Run("should skip internal commands", func(t *testing.T) {
-		for _, name := range []string{"completion", "help", "lsp", "self"} {
-			if shouldCheckForUpdate(name, true, defaultSettings) {
-				t.Fatalf("expected %q to skip update check", name)
+	t.Run("should skip completion and help commands", func(t *testing.T) {
+		for _, command := range []*cobra.Command{{Use: "completion"}, {Use: "help"}} {
+			if shouldCheckForUpdate(command, true, defaultSettings) {
+				t.Fatalf("expected %q to skip update check", command.Name())
 			}
 		}
 	})
+
+	t.Run("should skip self subcommands", func(t *testing.T) {
+		root := cmdpkg.CreateRootCommand("v0.0.0-test", "")
+		cmdpkg.InitSelfCmd(root, "v0.0.0-test")
+
+		for _, args := range [][]string{{"self"}, {"self", "doc"}, {"self", "upgrade"}} {
+			command, _, err := root.Find(args)
+			if err != nil {
+				t.Fatalf("unexpected error for %v: %v", args, err)
+			}
+
+			if shouldCheckForUpdate(command, true, defaultSettings) {
+				t.Fatalf("expected %v to skip update check", args)
+			}
+		}
+	})
+}
+
+func TestParseRootFlags(t *testing.T) {
+	t.Run("should reject legacy upgrade flag", func(t *testing.T) {
+		_, err := parseRootFlags([]string{"--upgrade"})
+		if err == nil {
+			t.Fatal("expected legacy upgrade flag error")
+		}
+
+		if err.Error() != "--upgrade has been replaced with 'lets self upgrade'" {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
 }

internal/cmd/root.go

@@ -98,7 +98,6 @@ func initRootFlags(rootCmd *cobra.Command) {
 	rootCmd.Flags().StringToStringP("env", "E", nil, "set env variable for running command KEY=VALUE")
 	rootCmd.Flags().StringArray("only", []string{}, "run only specified command(s) described in cmd as map")
 	rootCmd.Flags().StringArray("exclude", []string{}, "run all but excluded command(s) described in cmd as map")
-	rootCmd.Flags().Bool("upgrade", false, "upgrade lets to latest version")
 	rootCmd.Flags().Bool("init", false, "create a new lets.yaml in the current folder")
 	rootCmd.Flags().Bool("no-depends", false, "skip 'depends' for running command")
 	rootCmd.Flags().CountP("debug", "d", "show debug logs (or use LETS_DEBUG=1). If used multiple times, shows more verbose logs") //nolint:lll

internal/cmd/root_test.go

@@ -2,11 +2,13 @@ package cmd
 
 import (
 	"bytes"
+	"context"
 	"errors"
 	"strings"
 	"testing"
 
 	"github.com/lets-cli/lets/internal/config/config"
+	"github.com/lets-cli/lets/internal/upgrade"
 	"github.com/spf13/cobra"
 )
 
@@ -298,4 +300,71 @@ func TestSelfCmd(t *testing.T) {
 			t.Fatalf("expected no suggestions, got %q", err.Error())
 		}
 	})
+
+	t.Run("should run self upgrade subcommand", func(t *testing.T) {
+		bufOut := new(bytes.Buffer)
+		called := false
+
+		rootCmd := CreateRootCommand("v0.0.0-test", "")
+		rootCmd.SetArgs([]string{"self", "upgrade"})
+		rootCmd.SetOut(bufOut)
+		rootCmd.SetErr(bufOut)
+		selfCmd := &cobra.Command{
+			Use:   "self",
+			Short: "Manage lets CLI itself",
+		}
+		rootCmd.AddCommand(selfCmd)
+
+		selfCmd.AddCommand(initUpgradeCommandWith(func() (upgrade.Upgrader, error) {
+			return mockUpgraderFunc(func(ctx context.Context) error {
+				called = true
+
+				return nil
+			}), nil
+		}))
+
+		err := rootCmd.Execute()
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		if !called {
+			t.Fatal("expected upgrader to be called")
+		}
+	})
+
+	t.Run("should return upgrader error for self upgrade command", func(t *testing.T) {
+		bufOut := new(bytes.Buffer)
+
+		rootCmd := CreateRootCommand("v0.0.0-test", "")
+		rootCmd.SetArgs([]string{"self", "upgrade"})
+		rootCmd.SetOut(bufOut)
+		rootCmd.SetErr(bufOut)
+		selfCmd := &cobra.Command{
+			Use:   "self",
+			Short: "Manage lets CLI itself",
+		}
+		rootCmd.AddCommand(selfCmd)
+
+		selfCmd.AddCommand(initUpgradeCommandWith(func() (upgrade.Upgrader, error) {
+			return mockUpgraderFunc(func(ctx context.Context) error {
+				return errors.New("upgrade failed")
+			}), nil
+		}))
+
+		err := rootCmd.Execute()
+		if err == nil {
+			t.Fatal("expected upgrader error")
+		}
+
+		if !strings.Contains(err.Error(), "can not self-upgrade binary") {
+			t.Fatalf("expected self-upgrade error, got %q", err.Error())
+		}
+	})
+}
+
+type mockUpgraderFunc func(context.Context) error
+
+func (f mockUpgraderFunc) Upgrade(ctx context.Context) error {
+	return f(ctx)
 }

internal/cmd/self.go

@@ -26,4 +26,5 @@ func initSelfCmd(rootCmd *cobra.Command, version string, openURL func(string) er
 
 	selfCmd.AddCommand(initDocCommand(openURL))
 	selfCmd.AddCommand(initLspCommand(version))
+	selfCmd.AddCommand(initUpgradeCommand(version))
 }

internal/cmd/upgrade.go

@@ -0,0 +1,39 @@
+package cmd
+
+import (
+	"fmt"
+
+	"github.com/lets-cli/lets/internal/upgrade"
+	"github.com/lets-cli/lets/internal/upgrade/registry"
+	"github.com/spf13/cobra"
+)
+
+type upgraderFactory func() (upgrade.Upgrader, error)
+
+func initUpgradeCommand(version string) *cobra.Command {
+	return initUpgradeCommandWith(func() (upgrade.Upgrader, error) {
+		return upgrade.NewBinaryUpgrader(registry.NewGithubRegistry(), version)
+	})
+}
+
+func initUpgradeCommandWith(createUpgrader upgraderFactory) *cobra.Command {
+	upgradeCmd := &cobra.Command{
+		Use:   "upgrade",
+		Short: "Upgrade lets to latest version",
+		Args:  cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			upgrader, err := createUpgrader()
+			if err != nil {
+				return fmt.Errorf("can not self-upgrade binary: %w", err)
+			}
+
+			if err := upgrader.Upgrade(cmd.Context()); err != nil {
+				return fmt.Errorf("can not self-upgrade binary: %w", err)
+			}
+
+			return nil
+		},
+	}
+
+	return upgradeCmd
+}