Add logic to show user survey in DisplaySurveyPrompt flow. (#6196)

* add survey config and -id flag to survey command * add tests * revert not required * code review comments * move recentlyPromptedOrTaken and isSurveyPromptDisabled to survey. Create a new timeutil package * code review comments * fix rebase * refactor: Use globalConfig instead of kubecontext config for survey config. * Don't update survey prompt if survey prompt is not shown to stdout * fix linter * refactor UpdateGlobalSurveyTaken * survey is only active if start date is in past * Add global config for user surveys. Add logic to display user prompts * Apply suggestions from code review Co-authored-by: Brian de Alwis <bsd@acm.org> * code review changes * lint func * address nil pointer exception Co-authored-by: Brian de Alwis <bsd@acm.org>
GoogleContainerTools · Jul 13, 2021 · b3e0201 · b3e0201
1 parent 6391c38
commit b3e0201
Show file tree

Hide file tree

Showing 9 changed files with 400 additions and 30 deletions.
diff --git a/cmd/skaffold/app/cmd/cmd.go b/cmd/skaffold/app/cmd/cmd.go
@@ -63,9 +63,9 @@ const (
 
 func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
 	updateMsg := make(chan string, 1)
-	surveyPrompt := make(chan bool, 1)
+	surveyPrompt := make(chan string, 1)
 	var metricsPrompt bool
-	s := survey.New(opts.GlobalConfig)
+	var s *survey.Runner
 
 	rootCmd := &cobra.Command{
 		Use: "skaffold",
@@ -111,10 +111,11 @@ func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
 				logrus.Debugf("Disable housekeeping messages for command explicitly")
 				return nil
 			}
+			s = survey.New(opts.GlobalConfig, opts.ConfigurationFile, opts.Command)
 			// Always perform all checks.
 			go func() {
 				updateMsg <- updateCheckForReleasedVersionsIfNotDisabled(versionInfo.Version)
-				surveyPrompt <- s.ShouldDisplaySurveyPrompt()
+				surveyPrompt <- s.NextSurveyID()
 			}()
 			metricsPrompt = prompt.ShouldDisplayMetricsPrompt(opts.GlobalConfig)
 			return nil
@@ -133,9 +134,9 @@ func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
 			}
 			// check if survey prompt needs to be displayed
 			select {
-			case shouldDisplay := <-surveyPrompt:
-				if shouldDisplay {
-					if err := s.DisplaySurveyPrompt(cmd.OutOrStdout()); err != nil {
+			case promptSurveyID := <-surveyPrompt:
+				if promptSurveyID != "" {
+					if err := s.DisplaySurveyPrompt(cmd.OutOrStdout(), promptSurveyID); err != nil {
 						fmt.Fprintf(cmd.OutOrStderr(), "%v\n", err)
 					}
 				}

diff --git a/cmd/skaffold/app/cmd/survey.go b/cmd/skaffold/app/cmd/survey.go
@@ -37,6 +37,6 @@ func NewCmdSurvey() *cobra.Command {
 }
 
 func showSurvey(ctx context.Context, out io.Writer) error {
-	s := survey.New(opts.GlobalConfig)
+	s := survey.New(opts.GlobalConfig, opts.ConfigurationFile, opts.Command)
 	return s.OpenSurveyForm(ctx, out, surveyID)
 }
diff --git a/pkg/skaffold/config/global_config.go b/pkg/skaffold/config/global_config.go
@@ -42,9 +42,15 @@ type ContextConfig struct {
 
 // SurveyConfig is the survey config information
 type SurveyConfig struct {
-	DisablePrompt *bool  `yaml:"disable-prompt,omitempty"`
-	LastTaken     string `yaml:"last-taken,omitempty"`
-	LastPrompted  string `yaml:"last-prompted,omitempty"`
+	DisablePrompt *bool         `yaml:"disable-prompt,omitempty"`
+	LastTaken     string        `yaml:"last-taken,omitempty"`
+	LastPrompted  string        `yaml:"last-prompted,omitempty"`
+	UserSurveys   []*UserSurvey `yaml:"user-surveys,omitempty"`
+}
+
+type UserSurvey struct {
+	ID    string `yaml:"id"`
+	Taken *bool  `yaml:"taken,omitempty"`
 }
 
 // UpdateConfig is the update config information

diff --git a/pkg/skaffold/config/util.go b/pkg/skaffold/config/util.go
@@ -58,6 +58,7 @@ var (
 
 	// update global config with the time the survey was last taken
 	updateLastTaken = "skaffold config set --survey --global last-taken %s"
+	updateUserTaken = "skaffold config set --survey --global --id %s taken true"
 	// update global config with the time the survey was last prompted
 	updateLastPrompted = "skaffold config set --survey --global last-prompted %s"
 )
@@ -434,3 +435,38 @@ func WriteFullConfig(configFile string, cfg *GlobalConfig) error {
 	}
 	return nil
 }
+
+func UpdateUserSurveyTaken(configFile string, id string) error {
+	ai := fmt.Sprintf(updateUserTaken, id)
+	aiErr := fmt.Errorf("could not automatically update the survey prompted timestamp - please run `%s`", ai)
+	configFile, err := ResolveConfigFile(configFile)
+	if err != nil {
+		return aiErr
+	}
+	fullConfig, err := ReadConfigFile(configFile)
+	if err != nil {
+		return aiErr
+	}
+	if fullConfig.Global == nil {
+		fullConfig.Global = &ContextConfig{}
+	}
+	if fullConfig.Global.Survey == nil {
+		fullConfig.Global.Survey = &SurveyConfig{}
+	}
+	fullConfig.Global.Survey.UserSurveys = updatedUserSurveys(fullConfig.Global.Survey.UserSurveys, id)
+	err = WriteFullConfig(configFile, fullConfig)
+	if err != nil {
+		return aiErr
+	}
+	return nil
+}
+
+func updatedUserSurveys(us []*UserSurvey, id string) []*UserSurvey {
+	for _, s := range us {
+		if s.ID == id {
+			s.Taken = util.BoolPtr(true)
+			return us
+		}
+	}
+	return append(us, &UserSurvey{ID: id, Taken: util.BoolPtr(true)})
+}
diff --git a/pkg/skaffold/config/util_test.go b/pkg/skaffold/config/util_test.go
@@ -710,3 +710,77 @@ func TestShouldDisplayUpdateMsg(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdateUserSurveyTaken(t *testing.T) {
+	tests := []struct {
+		description string
+		cfg         string
+		id          string
+		expectedCfg *GlobalConfig
+	}{
+		{
+			description: "update global context when user survey is empty",
+			id:          "foo",
+			expectedCfg: &GlobalConfig{
+				Global: &ContextConfig{
+					Survey: &SurveyConfig{UserSurveys: []*UserSurvey{
+						{ID: "foo", Taken: util.BoolPtr(true)},
+					}}},
+				ContextConfigs: []*ContextConfig{},
+			},
+		},
+		{
+			description: "append user survey when not nil",
+			cfg: `
+global:
+  survey:
+    user-surveys:
+      - id: "foo1"
+        taken: true
+kubeContexts: []`,
+			id: "foo2",
+			expectedCfg: &GlobalConfig{
+				Global: &ContextConfig{
+					Survey: &SurveyConfig{
+						UserSurveys: []*UserSurvey{
+							{ID: "foo1", Taken: util.BoolPtr(true)},
+							{ID: "foo2", Taken: util.BoolPtr(true)},
+						}}},
+				ContextConfigs: []*ContextConfig{},
+			},
+		},
+		{
+			description: "update entry for a key in user survey",
+			cfg: `
+global:
+  survey:
+    user-surveys:
+      - id: "foo" 
+        taken: false
+kubeContexts: []`,
+			id: "foo",
+			expectedCfg: &GlobalConfig{
+				Global: &ContextConfig{
+					Survey: &SurveyConfig{
+						UserSurveys: []*UserSurvey{
+							{ID: "foo", Taken: util.BoolPtr(true)},
+						}}},
+				ContextConfigs: []*ContextConfig{},
+			},
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			cfg := t.TempFile("config", []byte(test.cfg))
+			t.Override(&ReadConfigFile, ReadConfigFileNoCache)
+
+			// update the time
+			err := UpdateUserSurveyTaken(cfg, test.id)
+			t.CheckNoError(err)
+
+			actualConfig, cfgErr := ReadConfigFile(cfg)
+			t.CheckNoError(cfgErr)
+			t.CheckDeepEqual(test.expectedCfg, actualConfig)
+		})
+	}
+}
diff --git a/pkg/skaffold/survey/config.go b/pkg/skaffold/survey/config.go
@@ -18,9 +18,11 @@ package survey
 
 import (
 	"fmt"
+	"sort"
 	"time"
 
 	sConfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
 )
 
@@ -39,7 +41,29 @@ var (
 		URL: hatsURL,
 	}
 	// surveys contains all the skaffold survey information
-	surveys = []config{hats}
+	surveys = []config{
+		hats,
+		{
+			id:       "helm",
+			URL:      "https://forms.gle/cLQg8sGD71JnPSZf6",
+			startsAt: time.Date(2021, time.July, 15, 0, 0, 0, 0, time.UTC),
+			expiresAt: time.Date(2021, time.August,
+				14, 00, 00, 00, 0, time.UTC),
+			promptText: "Help improve Skaffold's Helm support by taking our 2-minute anonymous survey",
+			isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool {
+				for _, cfg := range cfgs {
+					v1Cfg, ok := cfg.(*latestV1.SkaffoldConfig)
+					if !ok {
+						return false
+					}
+					if h := v1Cfg.Deploy.HelmDeploy; h != nil {
+						return true
+					}
+				}
+				return false
+			},
+		},
+	}
 )
 
 // config defines a survey.
@@ -69,7 +93,7 @@ func (s config) prompt() string {
 		return fmt.Sprintf(`%s: run 'skaffold survey'
 `, s.promptText)
 	}
-	return fmt.Sprintf(`%s: run 'skaffold survey -id %s'
+	return fmt.Sprintf(`%s: run 'skaffold survey --id %s'
 `, s.promptText, s.id)
 }
 
@@ -112,3 +136,19 @@ func init() {
 		}
 	}
 }
+
+// sortSurveys sorts a slice of config based on expiry time in
+// the ascending order.
+// Survey that don't have expiry set are returned last.
+func sortSurveys(s []config) []config {
+	sort.Slice(s, func(i, j int) bool {
+		if s[i].expiresAt.IsZero() { // i > j
+			return false
+		}
+		if s[j].expiresAt.IsZero() { // i < j
+			return true
+		}
+		return s[i].expiresAt.Before(s[j].expiresAt) // i < j
+	})
+	return s
+}
diff --git a/pkg/skaffold/survey/config_test.go b/pkg/skaffold/survey/config_test.go
@@ -44,7 +44,7 @@ func TestSurveyPrompt(t *testing.T) {
 				promptText: "Looks like you are using foo feature. Help improve Skaffold foo feature and take this survey",
 				expiresAt:  time.Date(2021, time.August, 14, 00, 00, 00, 0, time.UTC),
 			},
-			expected: `Looks like you are using foo feature. Help improve Skaffold foo feature and take this survey: run 'skaffold survey -id foo'
+			expected: `Looks like you are using foo feature. Help improve Skaffold foo feature and take this survey: run 'skaffold survey --id foo'
 `,
 		},
 	}
@@ -252,6 +252,48 @@ func TestIsValid(t *testing.T) {
 	}
 }
 
+func TestSortSurveys(t *testing.T) {
+	expected := []config{
+		{id: "10Day", expiresAt: time.Now().AddDate(0, 0, 10)},
+		{id: "started", startsAt: time.Now().AddDate(0, 0, -10), expiresAt: time.Now().AddDate(0, 0, 20)},
+		{id: "2Months", expiresAt: time.Now().AddDate(0, 2, 0)},
+		hats,
+	}
+	tests := []struct {
+		description string
+		input       []config
+	}{
+		{
+			description: "no expiry set at 0th position",
+			input: []config{
+				hats,
+				{id: "2Months", expiresAt: time.Now().AddDate(0, 2, 0)},
+				{id: "10Day", expiresAt: time.Now().AddDate(0, 0, 10)},
+				{id: "started", startsAt: time.Now().AddDate(0, 0, -10), expiresAt: time.Now().AddDate(0, 0, 20)},
+			},
+		},
+		{
+			description: "no expiry set in middle position",
+			input: []config{
+				{id: "started", startsAt: time.Now().AddDate(0, 0, -10), expiresAt: time.Now().AddDate(0, 0, 20)},
+				{id: "2Months", expiresAt: time.Now().AddDate(0, 2, 0)},
+				hats,
+				{id: "10Day", expiresAt: time.Now().AddDate(0, 0, 10)},
+			},
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			for i, a := range sortSurveys(test.input) {
+				if expected[i].id != a.id {
+					t.Errorf("expected to see %s, found %s at position %d",
+						expected[i].id, a.id, i)
+				}
+			}
+		})
+	}
+}
+
 // mockVersionedConfig implements util.VersionedConfig.
 type mockVersionedConfig struct {
 	version string