Show update check to users once in 24 hours (#5795)

GoogleContainerTools · May 6, 2021 · 36395cc · 36395cc
1 parent d4bdf6b
commit 36395cc
Show file tree

Hide file tree

Showing 5 changed files with 153 additions and 8 deletions.
diff --git a/cmd/skaffold/app/cmd/cmd.go b/cmd/skaffold/app/cmd/cmd.go
@@ -131,6 +131,9 @@ func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
 		PersistentPostRun: func(cmd *cobra.Command, args []string) {
 			select {
 			case msg := <-updateMsg:
+				if err := config.UpdateMsgDisplayed(opts.GlobalConfig); err != nil {
+					logrus.Debugf("could not update the 'last-prompted' config for 'update-config' section due to %s", err)
+				}
 				fmt.Fprintf(cmd.OutOrStderr(), "%s\n", msg)
 			default:
 			}

diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go
@@ -37,6 +37,7 @@ type ContextConfig struct {
 	KindDisableLoad      *bool         `yaml:"kind-disable-load,omitempty"`
 	K3dDisableLoad       *bool         `yaml:"k3d-disable-load,omitempty"`
 	CollectMetrics       *bool         `yaml:"collect-metrics,omitempty"`
+	UpdateCheckConfig    *UpdateConfig `yaml:"update,omitempty"`
 }
 
 // SurveyConfig is the survey config information
@@ -45,3 +46,9 @@ type SurveyConfig struct {
 	LastTaken     string `yaml:"last-taken,omitempty"`
 	LastPrompted  string `yaml:"last-prompted,omitempty"`
 }
+
+// UpdateConfig is the update config information
+type UpdateConfig struct {
+	// TODO (tejaldesai) Move ContextConfig.UpdateCheck config within this struct
+	LastPrompted string `yaml:"last-prompted,omitempty"`
+}
diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go
@@ -324,6 +324,42 @@ func recentlyPromptedOrTaken(cfg *ContextConfig) bool {
 	return lessThan(cfg.Survey.LastTaken, 90*24*time.Hour) || lessThan(cfg.Survey.LastPrompted, 10*24*time.Hour)
 }
 
+func ShouldDisplayUpdateMsg(configfile string) bool {
+	cfg, err := GetConfigForCurrentKubectx(configfile)
+	if err != nil {
+		return true
+	}
+	if cfg == nil || cfg.UpdateCheckConfig == nil {
+		return true
+	}
+	return !lessThan(cfg.UpdateCheckConfig.LastPrompted, 24*time.Hour)
+}
+
+// UpdateMsgDisplayed updates the `last-prompted` config for `update-config` in
+// the skaffold config
+func UpdateMsgDisplayed(configFile string) error {
+	// Today's date
+	today := current().Format(time.RFC3339)
+
+	configFile, err := ResolveConfigFile(configFile)
+	if err != nil {
+		return err
+	}
+	fullConfig, err := ReadConfigFile(configFile)
+	if err != nil {
+		return err
+	}
+	if fullConfig.Global == nil {
+		fullConfig.Global = &ContextConfig{}
+	}
+	if fullConfig.Global.UpdateCheckConfig == nil {
+		fullConfig.Global.UpdateCheckConfig = &UpdateConfig{}
+	}
+	fullConfig.Global.UpdateCheckConfig.LastPrompted = today
+	err = WriteFullConfig(configFile, fullConfig)
+	return err
+}
+
 func lessThan(date string, duration time.Duration) bool {
 	t, err := time.Parse(time.RFC3339, date)
 	if err != nil {

diff --git a/pkg/skaffold/config/util_test.go b/pkg/skaffold/config/util_test.go
@@ -764,3 +764,102 @@ kubeContexts: []`,
 		})
 	}
 }
+
+func TestUpdateMsgDisplayed(t *testing.T) {
+	testTimeStr := "2021-01-01T00:00:00Z"
+	tests := []struct {
+		description string
+		cfg         string
+		expectedCfg *GlobalConfig
+	}{
+		{
+			description: "update global context when context is empty",
+			expectedCfg: &GlobalConfig{
+				Global: &ContextConfig{
+					UpdateCheckConfig: &UpdateConfig{LastPrompted: testTimeStr},
+				},
+				ContextConfigs: []*ContextConfig{},
+			},
+		},
+		{
+			description: "update global context when update config is not nil",
+			cfg: `
+global:
+  update-config:
+    last-prompted: "some date"
+kubeContexts: []`,
+			expectedCfg: &GlobalConfig{
+				Global: &ContextConfig{
+					UpdateCheckConfig: &UpdateConfig{LastPrompted: testTimeStr},
+				},
+				ContextConfigs: []*ContextConfig{},
+			},
+		},
+		{
+			description: "update global context when update config last taken is in past",
+			cfg: `
+global:
+  update-config:
+    last-taken: "some date in past"
+kubeContexts: []`,
+			expectedCfg: &GlobalConfig{
+				Global: &ContextConfig{
+					UpdateCheckConfig: &UpdateConfig{
+						LastPrompted: "2021-01-01T00:00:00Z"},
+				},
+				ContextConfigs: []*ContextConfig{},
+			},
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			configFile := t.TempFile("config", []byte(test.cfg))
+			t.Override(&ReadConfigFile, ReadConfigFileNoCache)
+			t.Override(&current, func() time.Time {
+				testTime, _ := time.Parse(time.RFC3339, testTimeStr)
+				return testTime
+			})
+
+			// update the cfg
+			err := UpdateMsgDisplayed(configFile)
+			t.CheckNoError(err)
+
+			cfg, cfgErr := ReadConfigFileNoCache(configFile)
+			t.CheckErrorAndDeepEqual(false, cfgErr, test.expectedCfg, cfg)
+		})
+	}
+}
+
+func TestShouldDisplayUpdateMsg(t *testing.T) {
+	today, _ := time.Parse(time.RFC3339, "2021-01-01T12:04:05Z")
+	todayStr := "2021-01-01T00:00:00Z"
+	yesterday := "2020-12-22T00:00:00Z"
+	tests := []struct {
+		description string
+		cfg         *ContextConfig
+		expected    bool
+	}{
+		{
+			description: "should not display prompt when prompt is displayed in last 24 hours",
+			cfg: &ContextConfig{
+				UpdateCheckConfig: &UpdateConfig{LastPrompted: todayStr},
+			},
+		},
+		{
+			description: "should display prompt when last prompted is before 24 hours",
+			cfg: &ContextConfig{
+				UpdateCheckConfig: &UpdateConfig{LastPrompted: yesterday},
+			},
+			expected: true,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.Override(&GetConfigForCurrentKubectx, func(string) (*ContextConfig, error) { return test.cfg, nil })
+			t.Override(&current, func() time.Time {
+				return today
+			})
+			t.CheckDeepEqual(test.expected, ShouldDisplayUpdateMsg("dummyconfig"))
+		})
+	}
+}
diff --git a/pkg/skaffold/update/update.go b/pkg/skaffold/update/update.go
@@ -49,22 +49,22 @@ func CheckVersionOnError(config string) (string, error) {
 	return checkVersion(config, true)
 }
 
-func checkVersion(config string, onError bool) (string, error) {
-	if !isUpdateCheckEnabled(config) {
+func checkVersion(configfile string, onError bool) (string, error) {
+	if !isUpdateCheckEnabled(configfile) {
 		logrus.Debugf("Update check not enabled, skipping.")
 		return "", nil
 	}
 	latest, current, err := GetLatestAndCurrentVersion()
 	if err != nil {
 		return "", fmt.Errorf("getting latest and current skaffold versions: %w", err)
 	}
-	if !latest.GT(current) {
-		return "", nil
-	}
-	if onError {
-		return fmt.Sprintf("Your Skaffold version might be too old. Download the latest version (%s) from:\n  %s\n", latest, releaseURL(latest)), nil
+	if latest.GT(current) && config.ShouldDisplayUpdateMsg(configfile) {
+		if onError {
+			return fmt.Sprintf("Your Skaffold version might be too old. Download the latest version (%s) from:\n  %s\n", latest, releaseURL(latest)), nil
+		}
+		return fmt.Sprintf("There is a new version (%s) of Skaffold available. Download it from:\n  %s\n", latest, releaseURL(latest)), nil
 	}
-	return fmt.Sprintf("There is a new version (%s) of Skaffold available. Download it from:\n  %s\n", latest, releaseURL(latest)), nil
+	return "", nil
 }
 
 // isUpdateCheckEnabled returns whether or not the update check is enabled