Add logic to show user survey in DisplaySurveyPrompt flow. #6196
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6196 +/- ##
==========================================
+ Coverage 71.09% 71.22% +0.13%
==========================================
Files 485 481 -4
Lines 21609 21672 +63
==========================================
+ Hits 15362 15435 +73
+ Misses 5264 5253 -11
- Partials 983 984 +1
Continue to review full report at Codecov.
|
pkg/skaffold/config/util.go
Outdated
| 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) |
| return map[string]struct{}{} | ||
| } | ||
| taken := map[string]struct{}{} | ||
| if timeutil.LessThan(sc.LastTaken, 90*24*time.Hour) { |
There was a problem hiding this comment.
nit: extract these times as constants up top with description about what they control
There was a problem hiding this comment.
I actually had these as constants. I remember @tstromberg went in made them in to inline values here to follow the readability principle.
- define/declare variable code closer to where its used.
- dont create a constant if its not used multiple time.
I also do prefer constants, but he convinced me its easy to infer this value due to time.Hour
| expiresAt: time.Date(2021, time.August, | ||
| 14, 00, 00, 00, 0, time.UTC), | ||
| promptText: "Skaffold is working on revamping helm deployer support. Help us understand your helm deployer use case with our 2-minute anonymous survey", | ||
| isRelevantFn: func(cfgs []util.VersionedConfig, _ sConfig.RunMode) bool { |
There was a problem hiding this comment.
wdyt about replacing isRelevantFn with an explicit type survey.target where:
type target interface {
valid([]util.VersionedConfig, sConfig.RunMode) bool
}
type targets []target
func (t targets) valid(c []util.VersionedConfig, m sConfig.RunMode) bool{
for _, survey := range t {
if !survey.valid(c, m) {
return false
}
}
return true
}so we can arbitrarily compose them for more targeted surveys
surveys = []config{
{
id: helmUsers,
targets: usesHelm{},
},
{
id: multiConfigHelmUsers,
targets: targets([]target{usesHelm{}, isMultiConfig{}}),
}
}There was a problem hiding this comment.
sounds like a good idea. Shd we do this when we do have to re-use?
| } | ||
|
|
||
| func New(configFile string) *Runner { | ||
| func New(configFile string, skaffoldConfig string, mode string) *Runner { |
There was a problem hiding this comment.
It would be nice if we could avoid reloading the configs for the survey and in the runner. Perhaps we could have provider / loader that is passed here and into the runner so that we only load it once. This can wait for later.
pkg/skaffold/survey/survey.go
Outdated
| if cfg == nil || cfg.Global == nil || cfg.Global.Survey == nil { | ||
| return false | ||
| return s.taken(cfg.Global.Survey) |
There was a problem hiding this comment.
This will result in a nil panic given the if condition above.
There was a problem hiding this comment.
s.taken first line has a check to make sure cfg.Global.Survey is nil and a test is added.
There was a problem hiding this comment.
update: not s.taken but function calls below. There is a nil input test
func (s *Runner) taken(gc *sConfig.SurveyConfig) bool {
// fetch candidate surveys not taken by the user.
s.surveyID = s.selectSurvey(gc)
return s.surveyID == ""
}
func (s *Runner) selectSurvey(gc *sConfig.SurveyConfig) string {
taken := surveysTaken(gc)
...
}
func surveysTaken(sc *sConfig.SurveyConfig) map[string]struct{} {
if sc == nil {
return map[string]struct{}{}
}
There was a problem hiding this comment.
hmm. Integration tests failed due to npe.
I am still scratching my brain why this happened.
Changed in 00b6cf4
Co-authored-by: Brian de Alwis <bsd@acm.org>
This is the meaty PR to
should merge after #6191, #6192, #6193, #6194
The above all PRs are small refactors.
Design document: #6186
Changes:
Change
surveyPromptfrom bool to string in cmd.Rename
survey.ShouldDisplaySurveyPrompt()tosurvey.SurveyPromptIDnow return a stringAdd
UpdateUserSurveyTakento updatecfg.Global.Survey.UserSurveyswith taken true once survey is takenFinally
recentlyPromptedOrTakennow does the followinga) find candidate user or hats survey id to present iff the survey prompt was not recently sown
Follow up work:
skaffold config set --survey --global --id helm taken trueso in case skaffold fails to update the config, users can manually run the command mentioned in aiErrTesting Notes
skaffold buildon helm deployer example - start date in futureskaffold buildon helm deployer example - start date in pastRunning the command