diff --git a/cmd/skaffold/app/cmd/survey.go b/cmd/skaffold/app/cmd/survey.go index cc4c8214d96..62d9800157c 100644 --- a/cmd/skaffold/app/cmd/survey.go +++ b/cmd/skaffold/app/cmd/survey.go @@ -25,13 +25,18 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/survey" ) +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) } -func showSurvey(context context.Context, out io.Writer) error { +func showSurvey(ctx context.Context, out io.Writer) error { s := survey.New(opts.GlobalConfig) - return s.OpenSurveyForm(context, out) + return s.OpenSurveyForm(ctx, out, surveyID) } diff --git a/docs/content/en/docs/references/cli/_index.md b/docs/content/en/docs/references/cli/_index.md index e5f97fb8a65..d8cb7c3ad6d 100644 --- a/docs/content/en/docs/references/cli/_index.md +++ b/docs/content/en/docs/references/cli/_index.md @@ -1125,6 +1125,9 @@ Opens a web browser to fill out the Skaffold survey ``` +Options: + --id='hats': Survey ID for survey command to open. + Usage: skaffold survey [options] @@ -1132,6 +1135,9 @@ Use "skaffold options" for a list of global command-line options (applies to all ``` +Env vars: + +* `SKAFFOLD_ID` (same as `--id`) ### skaffold test diff --git a/pkg/skaffold/constants/constants.go b/pkg/skaffold/constants/constants.go index aef384ae448..c512178caff 100644 --- a/pkg/skaffold/constants/constants.go +++ b/pkg/skaffold/constants/constants.go @@ -73,6 +73,9 @@ const ( GithubIssueLink = "https://github.com/GoogleContainerTools/skaffold/issues/new" Windows = "windows" + + // HaTS is the HaTS Survey ID + HaTS = "hats" ) type Phase string diff --git a/pkg/skaffold/survey/config.go b/pkg/skaffold/survey/config.go new file mode 100644 index 00000000000..2997766f394 --- /dev/null +++ b/pkg/skaffold/survey/config.go @@ -0,0 +1,113 @@ +/* +Copyright 2021 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package survey + +import ( + "fmt" + "time" + + 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: 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} +) + +// config defines a survey. +type config struct { + 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, sConfig.RunMode) bool + URL string +} + +func (s config) isActive() bool { + return s.expiresAt.IsZero() || s.expiresAt.After(time.Now()) +} + +func (s config) prompt() string { + if s.id == hats.id { + 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, 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) { + for _, s := range surveys { + if s.id == id { + return s, true + } + } + return config{}, false +} + +func validKeys() []string { + keys := make([]string, 0, len(surveys)) + for _, s := range surveys { + keys = append(keys, s.id) + } + 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)) + } + } +} diff --git a/pkg/skaffold/survey/config_test.go b/pkg/skaffold/survey/config_test.go new file mode 100644 index 00000000000..720decf3550 --- /dev/null +++ b/pkg/skaffold/survey/config_test.go @@ -0,0 +1,233 @@ +/* +Copyright 2021 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package survey + +import ( + "testing" + "time" + + sConfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util" + "github.com/GoogleContainerTools/skaffold/testutil" +) + +func TestSurveyPrompt(t *testing.T) { + tests := []struct { + description string + s config + expected string + }{ + { + description: "hats survey", + s: hats, + expected: `Help improve Skaffold with our 2-minute anonymous survey: run 'skaffold survey' +`, + }, + { + description: "not hats survey", + s: config{ + id: "foo", + 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' +`, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + t.CheckDeepEqual(test.s.prompt(), test.expected) + }) + } +} + +func TestSurveyActive(t *testing.T) { + tests := []struct { + description string + s config + expected bool + }{ + { + description: "no expiry", + s: hats, + expected: true, + }, + { + description: "expiry in past", + s: config{ + id: "expired", + expiresAt: time.Date(2020, 8, 1, 0, 0, 0, 0, time.UTC), + }, + }, + { + description: "expiry in future", + s: config{ + id: "active", + expiresAt: time.Now().AddDate(1, 0, 0), + }, + expected: true, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + t.CheckDeepEqual(test.s.isActive(), test.expected) + }) + } +} + +func TestSurveyRelevant(t *testing.T) { + testMock := mockVersionedConfig{version: "test"} + prodMock := mockVersionedConfig{version: "prod"} + + tests := []struct { + description string + s config + cfgs []util.VersionedConfig + expected bool + }{ + { + description: "hats is always relevant", + s: hats, + expected: true, + }, + { + description: "relevant based on input configs", + s: config{ + id: "foo", + isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool { + return len(cfgs) > 1 + }, + }, + cfgs: []util.VersionedConfig{testMock, prodMock}, + expected: true, + }, + { + description: "not relevant based on config", + s: config{ + id: "foo", + isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool { + return len(cfgs) > 1 + }, + }, + cfgs: []util.VersionedConfig{testMock}, + }, + { + description: "contains a config with test version", + s: config{ + id: "version-value-test", + isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool { + for _, cfg := range cfgs { + if m, ok := cfg.(mockVersionedConfig); ok { + if m.version == "test" { + return true + } + } + } + return false + }, + }, + cfgs: []util.VersionedConfig{prodMock, testMock}, + expected: true, + }, + { + description: "does not contains a config with test version", + s: config{ + id: "version-value-test", + isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool { + for _, cfg := range cfgs { + if m, ok := cfg.(mockVersionedConfig); ok { + if m.version == "test" { + return true + } + } + } + return false + }, + }, + cfgs: []util.VersionedConfig{prodMock}, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + 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) + }) + } +} + +// mockVersionedConfig implements util.VersionedConfig. +type mockVersionedConfig struct { + version string +} + +func (m mockVersionedConfig) GetVersion() string { + return m.version +} + +func (m mockVersionedConfig) Upgrade() (util.VersionedConfig, error) { + return m, nil +} diff --git a/pkg/skaffold/survey/survey.go b/pkg/skaffold/survey/survey.go index dab7dbdca1d..cd20d8eff5e 100644 --- a/pkg/skaffold/survey/survey.go +++ b/pkg/skaffold/survey/survey.go @@ -24,31 +24,26 @@ import ( "github.com/pkg/browser" "github.com/sirupsen/logrus" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" + sConfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/output" ) const ( - Prompt = `Help improve Skaffold with our 2-minute anonymous survey: run 'skaffold survey' -` + Form = `Thank you for offering your feedback on Skaffold! Understanding your experiences and opinions helps us make Skaffold better for you and other users. - URL = "https://forms.gle/BMTbGQXLWSdn7vEs6" -) - -var ( - Form = fmt.Sprintf(`Thank you for offering your feedback on Skaffold! Understanding your experiences and opinions helps us make Skaffold better for you and other users. - -Skaffold will now attempt to open the survey in your default web browser. You may also manually open it using this link: +Skaffold will now attempt to open the survey in your default web browser. You may also manually open it using this URL: %s Tip: To permanently disable the survey prompt, run: - skaffold config set --survey --global disable-prompt true`, URL) + skaffold config set --survey --global disable-prompt true` +) +var ( // for testing isStdOut = output.IsStdout open = browser.OpenURL - updateConfig = config.UpdateGlobalSurveyPrompted + updateConfig = sConfig.UpdateGlobalSurveyPrompted ) type Runner struct { @@ -63,21 +58,25 @@ func New(configFile string) *Runner { func (s *Runner) DisplaySurveyPrompt(out io.Writer) error { if isStdOut(out) { - output.Green.Fprintf(out, Prompt) + output.Green.Fprintf(out, hats.prompt()) } return updateConfig(s.configFile) } -func (s *Runner) OpenSurveyForm(_ context.Context, out io.Writer) error { - _, err := fmt.Fprintln(out, Form) +func (s *Runner) OpenSurveyForm(_ context.Context, out io.Writer, id string) error { + sc, ok := getSurvey(id) + if !ok { + return fmt.Errorf("invalid survey id %q - please enter one of %s", id, validKeys()) + } + _, err := fmt.Fprintln(out, fmt.Sprintf(Form, sc.URL)) if err != nil { return err } - if err := open(URL); err != nil { - logrus.Debugf("could not open url %s", URL) + if err := open(sc.URL); err != nil { + logrus.Debugf("could not open url %s", sc.URL) return err } // Currently we will only update the global survey taken // When prompting for the survey, we need to use the same field. - return config.UpdateGlobalSurveyTaken(s.configFile) + return sConfig.UpdateGlobalSurveyTaken(s.configFile) } diff --git a/pkg/skaffold/survey/survey_test.go b/pkg/skaffold/survey/survey_test.go index 58b83bc3fc9..fd5182831ac 100644 --- a/pkg/skaffold/survey/survey_test.go +++ b/pkg/skaffold/survey/survey_test.go @@ -33,7 +33,8 @@ func TestDisplaySurveyForm(t *testing.T) { { description: "std out", mockStdOut: true, - expected: Prompt, + expected: `Help improve Skaffold with our 2-minute anonymous survey: run 'skaffold survey' +`, }, { description: "not std out",