Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show update check message to users once in 24 hours #5795

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/skaffold/app/cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
}
Expand Down
7 changes: 7 additions & 0 deletions pkg/skaffold/config/global_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"`
}
36 changes: 36 additions & 0 deletions pkg/skaffold/config/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
99 changes: 99 additions & 0 deletions pkg/skaffold/config/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
})
}
}
16 changes: 8 additions & 8 deletions pkg/skaffold/update/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down