[version check] Avoid re-printing update notice in child devbox commands (#1007)

## Summary

Some devbox commands like `devbox shell` may invoke other devbox
commands. For example,
`devbox shell` generates a shellrc file that calls `devbox log`. This
results in
multiple "update version" notices being printed for the same devbox
command invocation.

To avoid this, we set an env-var after printing the notice, and unset
that env-var
when this devbox command will exit. Note, I keep this env-var inside
vercheck
to maintain encapsulation, instead of putting it in envir package.

I also pushed some of the skipping checks from `root.go` inside
`vercheck` for
(1) better encapsulation (2) adding test cases for them.

## How was it tested?

`go test ./internal/vercheck/...`

For manual testing:
1. `set -gx DEVBOX_LATEST_VERSION 0.5.0`
2. modify binary to `vercheck` to have `currentDevboxVersion = 0.4.8`
and `isDevBuild = false`.
3. run `devbox shell`
BEFORE: multiple update notices would print
AFTER: one notice was printed

Author: savil

internal/boxcli/root.go

@@ -11,8 +11,6 @@ import (
 	"strings"
 
 	"github.com/spf13/cobra"
-	"go.jetpack.io/devbox/internal/build"
-
 	"go.jetpack.io/devbox/internal/boxcli/midcobra"
 	"go.jetpack.io/devbox/internal/cloud/openssh/sshshim"
 	"go.jetpack.io/devbox/internal/debug"
@@ -38,16 +36,7 @@ func RootCmd() *cobra.Command {
 			if flags.quiet {
 				cmd.SetErr(io.Discard)
 			}
-
-			// Skip CheckVersion for `devbox global shellenv` because users will include that
-			// command in their shellrc files, and we don't want to bother them everytime they
-			// open their terminals.
-			//
-			// Skip CheckVersion for engineers building devbox during development.
-			if !strings.HasPrefix(cmd.CommandPath(), "devbox global shellenv") &&
-				!build.IsDev {
-				vercheck.CheckVersion(cmd.ErrOrStderr())
-			}
+			vercheck.CheckVersion(cmd.ErrOrStderr(), cmd.CommandPath())
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return cmd.Help()

internal/vercheck/vercheck.go

@@ -15,32 +15,60 @@ import (
 
 	"github.com/fatih/color"
 	"github.com/pkg/errors"
-	"golang.org/x/mod/semver"
-
+	"github.com/samber/lo"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
-	"go.jetpack.io/devbox/internal/build"
 	"go.jetpack.io/devbox/internal/cmdutil"
 	"go.jetpack.io/devbox/internal/envir"
 	"go.jetpack.io/devbox/internal/ux"
 	"go.jetpack.io/devbox/internal/xdg"
+	"golang.org/x/mod/semver"
 )
 
 // Keep this in-sync with latest version in launch.sh.
 // If this version is newer than the version in launch.sh, we'll print a notice.
 const expectedLauncherVersion = "v0.2.0"
 
+// envName determines whether the version check has already occurred.
+// We set this env-var so that this devbox command invoking other devbox commands
+// do not re-run the version check and print the notice again.
+const envName = "__DEVBOX_VERSION_CHECK"
+
 // currentDevboxVersion is the version of the devbox CLI binary that is currently running.
 // We use this variable so that we can mock it in tests.
-var currentDevboxVersion = build.Version
+var currentDevboxVersion = "0.4.8" // build.Version
+
+// isDevBuild determines whether this CLI binary was built during development, or published
+// as a release.
+// We use this variable so we can mock it in tests.
+var isDevBuild = false // build.IsDev
+
+var commandSkipList = []string{
+	"devbox global shellenv",
+	"devbox shellenv",
+}
 
 // CheckVersion checks the launcher and binary versions and prints a notice if
 // they are out of date.
-func CheckVersion(w io.Writer) {
+//
+// It will set the checkVersionEnvName to indicate that the version check was done.
+// Callers should call ClearEnvVar after their work is done.
+func CheckVersion(w io.Writer, commandPath string) {
+	if isDevBuild {
+		return
+	}
+
+	if os.Getenv(envName) == "1" {
+		return
+	}
 
 	if envir.IsDevboxCloud() {
 		return
 	}
 
+	if lo.Contains(commandSkipList, commandPath) {
+		return
+	}
+
 	// check launcher version
 	launcherNotice := launcherVersionNotice()
 	if launcherNotice != "" {
@@ -56,6 +84,8 @@ func CheckVersion(w io.Writer) {
 		// TODO: use ux.FNotice
 		color.New(color.FgYellow).Fprintf(w, devboxNotice)
 	}
+
+	os.Setenv(envName, "1")
 }
 
 // SelfUpdate updates the devbox launcher and devbox CLI binary.

internal/vercheck/vercheck_test.go

@@ -5,6 +5,7 @@ package vercheck
 
 import (
 	"bytes"
+	"os"
 	"strings"
 	"testing"
 
@@ -13,40 +14,46 @@ import (
 
 func TestCheckVersion(t *testing.T) {
 
-	t.Run("no_devbox_cloud", func(t *testing.T) {
+	isDevBuild = false
+
+	t.Run("skip_if_devbox_cloud", func(t *testing.T) {
+		defer os.Unsetenv(envName)
 		// if devbox cloud
 		t.Setenv(envir.DevboxRegion, "true")
 		buf := new(bytes.Buffer)
-		CheckVersion(buf)
+		CheckVersion(buf, "devbox shell")
 		if buf.String() != "" {
 			t.Errorf("expected empty string, got %q", buf.String())
 		}
 		t.Setenv(envir.DevboxRegion, "")
 	})
 
 	// no launcher version or latest-version env var
-	t.Run("no_launcher_version_or_latest_version", func(t *testing.T) {
+	t.Run("skip_if_no_launcher_version_or_latest_version", func(t *testing.T) {
+		defer os.Unsetenv(envName)
 		t.Setenv(envir.LauncherVersion, "")
 		t.Setenv(envir.DevboxLatestVersion, "")
 		buf := new(bytes.Buffer)
-		CheckVersion(buf)
+		CheckVersion(buf, "devbox shell")
 		if buf.String() != "" {
 			t.Errorf("expected empty string, got %q", buf.String())
 		}
 	})
 
-	t.Run("launcher_version_outdated", func(t *testing.T) {
+	t.Run("print_if_launcher_version_outdated", func(t *testing.T) {
+		defer os.Unsetenv(envName)
 		// set older launcher version
 		t.Setenv(envir.LauncherVersion, "v0.1.0")
 
 		buf := new(bytes.Buffer)
-		CheckVersion(buf)
+		CheckVersion(buf, "devbox shell")
 		if !strings.Contains(buf.String(), "New launcher available") {
 			t.Errorf("expected notice about new launcher version, got %q", buf.String())
 		}
 	})
 
-	t.Run("binary_version_outdated", func(t *testing.T) {
+	t.Run("print_if_binary_version_outdated", func(t *testing.T) {
+		defer os.Unsetenv(envName)
 		// set the launcher version so that it is not outdated
 		t.Setenv(envir.LauncherVersion, strings.TrimPrefix(expectedLauncherVersion, "v"))
 
@@ -57,13 +64,14 @@ func TestCheckVersion(t *testing.T) {
 		currentDevboxVersion = "v0.4.8"
 
 		buf := new(bytes.Buffer)
-		CheckVersion(buf)
+		CheckVersion(buf, "devbox shell")
 		if !strings.Contains(buf.String(), "New devbox available") {
 			t.Errorf("expected notice about new devbox version, got %q", buf.String())
 		}
 	})
 
-	t.Run("all_versions_up_to_date", func(t *testing.T) {
+	t.Run("skip_if_all_versions_up_to_date", func(t *testing.T) {
+		defer os.Unsetenv(envName)
 
 		// set the launcher version so that it is not outdated
 		t.Setenv(envir.LauncherVersion, strings.TrimPrefix(expectedLauncherVersion, "v"))
@@ -75,9 +83,44 @@ func TestCheckVersion(t *testing.T) {
 		t.Setenv(envir.DevboxLatestVersion, "0.4.8")
 
 		buf := new(bytes.Buffer)
-		CheckVersion(buf)
+		CheckVersion(buf, "devbox shell")
+		if buf.String() != "" {
+			t.Errorf("expected empty string, got %q", buf.String())
+		}
+	})
+
+	t.Run("skip_if_dev_build", func(t *testing.T) {
+		defer os.Unsetenv(envName)
+		isDevBuild = true
+		defer func() { isDevBuild = false }()
+
+		// set older launcher version
+		t.Setenv(envir.LauncherVersion, "v0.1.0")
+
+		buf := new(bytes.Buffer)
+		CheckVersion(buf, "devbox shell")
 		if buf.String() != "" {
 			t.Errorf("expected empty string, got %q", buf.String())
 		}
 	})
+
+	t.Run("skip_if_command_path_skipped", func(t *testing.T) {
+		defer os.Unsetenv(envName)
+
+		for _, cmdPath := range commandSkipList {
+			cmdPathUnderscored := strings.ReplaceAll(cmdPath, " ", "_")
+			t.Run("skip_if_cmd_path_is_"+cmdPathUnderscored, func(t *testing.T) {
+
+				// set older launcher version
+				t.Setenv(envir.LauncherVersion, "v0.1.0")
+
+				buf := new(bytes.Buffer)
+				CheckVersion(buf, cmdPath)
+				if buf.String() != "" {
+					t.Errorf("expected empty string, got %q", buf.String())
+				}
+			})
+		}
+	})
+
 }