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

Add survey config and framework to show feature surveys to skaffold users. #6185

Merged
merged 5 commits into from
Jul 12, 2021
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
9 changes: 7 additions & 2 deletions cmd/skaffold/app/cmd/survey.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
6 changes: 6 additions & 0 deletions docs/content/en/docs/references/cli/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions pkg/skaffold/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ const (
GithubIssueLink = "https://github.com/GoogleContainerTools/skaffold/issues/new"

Windows = "windows"

// HaTS is the HaTS Survey ID
HaTS = "hats"
tejal29 marked this conversation as resolved.
Show resolved Hide resolved
)

type Phase string
Expand Down
113 changes: 113 additions & 0 deletions pkg/skaffold/survey/config.go
Original file line number Diff line number Diff line change
@@ -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())
tejal29 marked this conversation as resolved.
Show resolved Hide resolved
}

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)
Comment on lines +71 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the trailing newline be left to the caller who is actually outputting to the console.

}

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))
}
}
}
233 changes: 233 additions & 0 deletions pkg/skaffold/survey/config_test.go
Original file line number Diff line number Diff line change
@@ -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
tejal29 marked this conversation as resolved.
Show resolved Hide resolved

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),
tejal29 marked this conversation as resolved.
Show resolved Hide resolved
},
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
}
Loading