From 20062233c8402d969da313d854903db7afa03379 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 10 Jan 2025 14:16:46 -0500 Subject: [PATCH] Separate logic for checking updates During discussion in cli/cli#9934, we can to the conclusion that the logic around checking for core GitHub CLI updates would diverge from GitHub CLI extension updates over time. To that end, this commit splits that logic into a separate function with a new environment variable. --- internal/update/update.go | 18 +++++++++-- internal/update/update_test.go | 57 ++++++++++++++++++++++++++++++++++ pkg/cmd/root/extension.go | 2 +- pkg/cmd/root/help_topic.go | 8 +++-- 4 files changed, 79 insertions(+), 6 deletions(-) diff --git a/internal/update/update.go b/internal/update/update.go index 1afabaf21..a4a15ea17 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -33,9 +33,10 @@ type StateEntry struct { LatestRelease ReleaseInfo `yaml:"latest_release"` } -// ShouldCheckForUpdate decides whether we check for updates for the GitHub CLI and/or GitHub CLI extensions based on user preferences and current execution context. -func ShouldCheckForUpdate() bool { - if os.Getenv("GH_NO_UPDATE_NOTIFIER") != "" { +// ShouldCheckForExtensionUpdate decides whether we check for updates for GitHub CLI extensions based on user preferences and current execution context. +// During cli/cli#9934, this logic was split out from ShouldCheckForUpdate() because we envisioned it going in a different direction. +func ShouldCheckForExtensionUpdate() bool { + if os.Getenv("GH_NO_EXTENSION_UPDATE_NOTIFIER") != "" { return false } if os.Getenv("CODESPACES") != "" { @@ -74,6 +75,17 @@ func CheckForExtensionUpdate(em extensions.ExtensionManager, ext extensions.Exte return nil, nil } +// ShouldCheckForUpdate decides whether we check for updates for the GitHub CLI based on user preferences and current execution context. +func ShouldCheckForUpdate() bool { + if os.Getenv("GH_NO_UPDATE_NOTIFIER") != "" { + return false + } + if os.Getenv("CODESPACES") != "" { + return false + } + return !IsCI() && IsTerminal(os.Stdout) && IsTerminal(os.Stderr) +} + // CheckForUpdate checks whether an update exists for the GitHub CLI based on recency of last check within past 24 hours. func CheckForUpdate(ctx context.Context, client *http.Client, stateFilePath, repo, currentVersion string) (*ReleaseInfo, error) { stateEntry, _ := getStateEntry(stateFilePath) diff --git a/internal/update/update_test.go b/internal/update/update_test.go index ebddee810..089e36f68 100644 --- a/internal/update/update_test.go +++ b/internal/update/update_test.go @@ -425,6 +425,63 @@ func TestShouldCheckForUpdate(t *testing.T) { } } +func TestShouldCheckForExtensionUpdate(t *testing.T) { + tests := []struct { + name string + env map[string]string + expected bool + }{ + { + name: "should not check when user has explicitly disable notifications", + env: map[string]string{ + "GH_NO_EXTENSION_UPDATE_NOTIFIER": "1", + }, + expected: false, + }, + { + name: "should not check when user is in codespace", + env: map[string]string{ + "CODESPACES": "1", + }, + expected: false, + }, + { + name: "should not check when in GitHub Actions / Travis / Circle / Cirrus / GitLab / AppVeyor / CodeShip / dsari", + env: map[string]string{ + "CI": "1", + }, + expected: false, + }, + { + name: "should not check when in Jenkins / TeamCity", + env: map[string]string{ + "BUILD_NUMBER": "1", + }, + expected: false, + }, + { + name: "should not check when in TaskCluster / dsari", + env: map[string]string{ + "RUN_ID": "1", + }, + expected: false, + }, + // TODO: Figure out how to refactor IsTerminal() to be testable + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + os.Clearenv() + for k, v := range tt.env { + os.Setenv(k, v) + } + + actual := ShouldCheckForExtensionUpdate() + require.Equal(t, tt.expected, actual) + }) + } +} + func tempFilePath() string { file, err := os.CreateTemp("", "") if err != nil { diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index b9cb3dfff..7f2325e18 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -77,7 +77,7 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex } func checkForExtensionUpdate(em extensions.ExtensionManager, ext extensions.Extension) (*update.ReleaseInfo, error) { - if !update.ShouldCheckForUpdate() { + if !update.ShouldCheckForExtensionUpdate() { return nil, nil } diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index 7cca4cb7a..4359e4212 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -86,8 +86,12 @@ var HelpTopics = []helpTopic{ available in the viewport. When the value is a percentage, it will be applied against the number of columns available in the current viewport. - %[1]sGH_NO_UPDATE_NOTIFIER%[1]s: set to any value to disable update notifications. By default, gh - checks for new releases once every 24 hours and displays an upgrade notice on standard + %[1]sGH_NO_UPDATE_NOTIFIER%[1]s: set to any value to disable GitHub CLI update notifications. + By default, gh checks for new releases once every 24 hours and displays an upgrade notice on standard + error if a newer version was found. + + %[1]sGH_NO_EXTENSION_UPDATE_NOTIFIER%[1]s: set to any value to disable GitHub CLI extension update notifications. + By default, gh checks for new releases once every 24 hours and displays an upgrade notice on standard error if a newer version was found. %[1]sGH_CONFIG_DIR%[1]s: the directory where gh will store configuration files. If not specified,