code review comments

Author: tejal29

cmd/skaffold/app/cmd/flags.go

@@ -573,14 +573,6 @@ var flagRegistry = []Flag{
 		},
 		IsEnum: true,
 	},
-	{
-		Name:          "id",
-		Usage:         "Survey id for survey command to open.",
-		Value:         &surveyID,
-		DefValue:      constants.HaTS,
-		FlagAddMethod: "StringVar",
-		DefinedOn:     []string{"survey"},
-	},
 }
 
 func methodNameByType(v reflect.Value) string {

cmd/skaffold/app/cmd/survey.go

@@ -30,6 +30,9 @@ var surveyID string
 func NewCmdSurvey() *cobra.Command {
 	return NewCmd("survey").
 		WithDescription("Opens a web browser to fill out the Skaffold survey").
+		WithFlags([]*Flag{
+			{Value: &surveyID, Name: "id", DefValue: survey.HatsID, Usage: "Survey ID for survey command to open."},
+		}).
 		NoArgs(showSurvey)
 }
 

docs/content/en/docs/references/cli/_index.md

@@ -1125,13 +1125,19 @@ Opens a web browser to fill out the Skaffold survey
 ```
 
 
+Options:
+      --id='hats': Survey ID for survey command to open.
+
 Usage:
   skaffold survey [options]
 
 Use "skaffold options" for a list of global command-line options (applies to all commands).
 
 
 ```
+Env vars:
+
+* `SKAFFOLD_ID` (same as `--id`)
 
 ### skaffold test
 

pkg/skaffold/survey/config.go

@@ -20,37 +20,42 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
+	sConfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
 )
 
 const (
+	HatsID  = "hats"
 	hatsURL = "https://forms.gle/BMTbGQXLWSdn7vEs6"
 )
 
 var (
 	hats = config{
-		id: constants.HaTS,
-		promptText: `Help improve Skaffold with our 2-minute anonymous survey: run 'skaffold survey'
-`,
-		isRelevantFn: func(_ []util.VersionedConfig) bool { return true },
-		URL:          hatsURL,
+		id:         HatsID,
+		promptText: "Help improve Skaffold with our 2-minute anonymous survey",
+		isRelevantFn: func([]util.VersionedConfig, sConfig.RunMode) bool {
+			return true
+		},
+		URL: hatsURL,
 	}
 	// surveys contains all the skaffold survey information
 	surveys = []config{hats}
-
-	// for testing
-	today = time.Now()
 )
 
 // config defines a survey.
 type config struct {
-	id           string
-
-	promptText   string
-
+	id string
+	// promptText is shown to the user and should be formatted so each line should fit in < 80 characters.
+	// For example: `As a Helm user, we are requesting your feedback on a proposed change to Skaffold's integration with Helm.`
+	promptText string
+	// startsAt mentions the date after the users survey should be prompted. This will ensure, Skaffold team can finalize the survey
+	// even after release date.
+	startsAt time.Time
+	// expiresAt places a time limit of the user survey. As users are only prompted every two weeks
+	// by design, this time limit should be at least 4 weeks after the upcoming release date to account
+	// for release propagation lag to Cloud SDK and Cloud Shell.
 	expiresAt    time.Time
-	isRelevantFn func([]util.VersionedConfig, ) bool
+	isRelevantFn func([]util.VersionedConfig, sConfig.RunMode) bool
 	URL          string
 }
 
@@ -60,14 +65,26 @@ func (s config) isActive() bool {
 
 func (s config) prompt() string {
 	if s.id == hats.id {
-		return s.promptText
+		return fmt.Sprintf(`%s: run 'skaffold survey'
+`, s.promptText)
 	}
 	return fmt.Sprintf(`%s: run 'skaffold survey -id %s'
 `, s.promptText, s.id)
 }
 
-func (s config) isRelevant(cfgs []util.VersionedConfig) bool {
-	return s.isRelevantFn(cfgs)
+func (s config) isRelevant(cfgs []util.VersionedConfig, cmd sConfig.RunMode) bool {
+	return s.isRelevantFn(cfgs, cmd)
+}
+
+func (s config) isValid() bool {
+	if s.id == HatsID {
+		return true
+	}
+	today := s.startsAt
+	if today.IsZero() {
+		today = time.Now()
+	}
+	return s.expiresAt.Sub(today) < 60*24*time.Hour
 }
 
 func getSurvey(id string) (config, bool) {
@@ -86,3 +103,11 @@ func validKeys() []string {
 	}
 	return keys
 }
+
+func init() {
+	for _, s := range surveys {
+		if !s.isValid() {
+			panic(fmt.Errorf("survey %q is valid for more than a 60 days - user surveys must be valid for 60 days or less", s.id))
+		}
+	}
+}

pkg/skaffold/survey/config_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 	"time"
 
+	sConfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
@@ -69,21 +70,20 @@ func TestSurveyActive(t *testing.T) {
 			description: "expiry in past",
 			s: config{
 				id:        "expired",
-				expiresAt: time.Date(2020, time.August, 14, 00, 00, 00, 0, time.UTC),
+				expiresAt: time.Date(2020, 8, 1, 0, 0, 0, 0, time.UTC),
 			},
 		},
 		{
 			description: "expiry in future",
 			s: config{
 				id:        "active",
-				expiresAt: time.Date(2022, time.August, 14, 00, 00, 00, 0, time.UTC),
+				expiresAt: time.Now().AddDate(1, 0, 0),
 			},
 			expected: true,
 		},
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			t.Override(&today, time.Date(2021, time.August, 14, 0, 0, 0, 0, time.UTC))
 			t.CheckDeepEqual(test.s.isActive(), test.expected)
 		})
 	}
@@ -108,7 +108,7 @@ func TestSurveyRelevant(t *testing.T) {
 			description: "relevant based on input configs",
 			s: config{
 				id: "foo",
-				isRelevantFn: func(cfgs []util.VersionedConfig) bool {
+				isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool {
 					return len(cfgs) > 1
 				},
 			},
@@ -119,7 +119,7 @@ func TestSurveyRelevant(t *testing.T) {
 			description: "not relevant based on config",
 			s: config{
 				id: "foo",
-				isRelevantFn: func(cfgs []util.VersionedConfig) bool {
+				isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool {
 					return len(cfgs) > 1
 				},
 			},
@@ -129,7 +129,7 @@ func TestSurveyRelevant(t *testing.T) {
 			description: "contains a config with test version",
 			s: config{
 				id: "version-value-test",
-				isRelevantFn: func(cfgs []util.VersionedConfig) bool {
+				isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool {
 					for _, cfg := range cfgs {
 						if m, ok := cfg.(mockVersionedConfig); ok {
 							if m.version == "test" {
@@ -147,7 +147,7 @@ func TestSurveyRelevant(t *testing.T) {
 			description: "does not contains a config with test version",
 			s: config{
 				id: "version-value-test",
-				isRelevantFn: func(cfgs []util.VersionedConfig) bool {
+				isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool {
 					for _, cfg := range cfgs {
 						if m, ok := cfg.(mockVersionedConfig); ok {
 							if m.version == "test" {
@@ -163,7 +163,58 @@ func TestSurveyRelevant(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			t.CheckDeepEqual(test.s.isRelevant(test.cfgs), test.expected)
+			t.CheckDeepEqual(test.s.isRelevant(test.cfgs, "dev"), test.expected)
+		})
+	}
+}
+
+func TestIsValid(t *testing.T) {
+	tests := []struct {
+		description string
+		s           config
+		expected    bool
+	}{
+		{
+			description: "only hats",
+			s:           hats,
+			expected:    true,
+		},
+		{
+			description: "4 weeks valid survey with start date",
+			s: config{
+				id:        "invalid",
+				startsAt:  time.Now().AddDate(0, 1, 0),
+				expiresAt: time.Now().AddDate(0, 2, 0),
+			},
+			expected: true,
+		},
+		{
+			description: "4 weeks valid survey without start date",
+			s: config{
+				id:        "valid",
+				expiresAt: time.Now().AddDate(0, 1, 0),
+			},
+			expected: true,
+		},
+		{
+			description: "90 days invalid survey without start date",
+			s: config{
+				id:        "invalid",
+				expiresAt: time.Now().AddDate(0, 0, 90),
+			},
+		},
+		{
+			description: "90 days invalid survey with start date",
+			s: config{
+				id:        "invalid",
+				startsAt:  time.Now().AddDate(0, 1, 0),
+				expiresAt: time.Now().AddDate(0, 1, 90),
+			},
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.CheckDeepEqual(test.s.isValid(), test.expected)
 		})
 	}
 }