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

hook up showing survey prompt if not taken or recently prompted #4306

Merged
merged 5 commits into from
Jun 10, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Jun 9, 2020

Fixes: #4046

In this PR, skaffold will show the survey prompt to users if

  1. The survey was not taken in last 3 months and
  2. The survey prompt was not show in last 2 weeks.

** UI Changes **

** Testing**

  1. Unit tests here cover all the below tests.

** Manual tests **

1. Survey not taken in last 3 months or not prompted recently
a. ~/.skaffold/config :

(add_survey_check)cat ~/.skaffold/config 
global:
  survey: {}
kubeContexts: []
 (add_survey_check)

b. Run any skaffold command

../../out/skaffold run -d gcr.io/tejal-test
Generating tags...
...
You can also run [skaffold run --tail] to get the logs
Help improve Skaffold! Take a 10-second anonymous survey by running
   skaffold survey
(add_survey_check)

2. Survey not taken in last 3 months but prompted recently
a. Skaffold config

(add_survey_check)cat ~/.skaffold/config 
global:
  survey:
    last-prompted: "2020-06-09T10:34:05-07:00"
kubeContexts: []

b. Run skaffold command and verify prompt is not shown.

(add_survey_check)../../out/skaffold run -d gcr.io/tejal-test
Generating tags...
....
You can also run [skaffold run --tail] to get the logs
 (add_survey_check)

3. Survey not taken in last 3 months but prompted but 2 weeks ago
a. set survey prompted taken to last month

(add_survey_check)skaffold config set --survey --global last-prompted "2020-05-15T10:34:05-07:00"
set global value last-prompted to 2020-05-15T10:34:05-07:00

b. Run any skaffold command and verify the prompt is shown.

(add_survey_check)./../out/skaffold run -d gcr.io/tejal-test
Generating tags...
....
You can also run [skaffold run --tail] to get the logs
Help improve Skaffold! Take a 10-second anonymous survey by running
   skaffold survey

4. Survey taken in last 3 months.
a. Set survey taken in last 3 months and not recently prompted

skaffold config set --survey --global last-taken "2020-04-30T10:34:05-07:00"
set global value last-taken to 2020-04-30T10:34:05-07:00
skaffold config unset --survey --global last-prompted
unset global value last-prompted
cat ~/.skaffold/config 
global:
  survey:
    last-taken: "2020-04-30T10:34:05-07:00"
kubeContexts: []
tejaldesai@tejaldesai-macbookpro2 getting-started (add_survey_check)

b. Run skaffold command and verify the prompt is not displayed

(add_survey_check)../../out/skaffold run -d gcr.io/tejal-test
Generating tags...
 ...
Deployments stabilized in 132.179878ms
You can also run [skaffold run --tail] to get the logs
tejaldesai@tejaldesai-macbookpro2 getting-started (add_survey_check)

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 9, 2020
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #4306 into master will decrease coverage by 0.01%.
The diff coverage is 59.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4306      +/-   ##
==========================================
- Coverage   71.93%   71.91%   -0.02%     
==========================================
  Files         322      322              
  Lines       12313    12341      +28     
==========================================
+ Hits         8857     8875      +18     
- Misses       2896     2902       +6     
- Partials      560      564       +4     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/cmd.go 67.64% <30.00%> (-1.89%) ⬇️
pkg/skaffold/config/util.go 71.26% <70.00%> (-0.35%) ⬇️
pkg/skaffold/survey/survey.go 52.94% <100.00%> (+21.69%) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 60.97% <0.00%> (-2.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7731b62...d51e032. Read the comment docs.

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

a few wording suggestions and one comment but in general LGTM

cmd/skaffold/app/cmd/cmd.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/cmd.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/cmd.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/cmd.go Outdated Show resolved Hide resolved
// Today's date
today := current().Format(time.RFC3339)
ai := fmt.Sprintf(updateLastPrompted, today)
aiErr := fmt.Errorf("could not automatically update the survey prompted timestamp - please run `%s`", ai)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this something we actually want the user to do? feels a little weird that we'd tell them to run a command to set a prompt timestamp - maybe we should tell them if this keeps happening to disable instead?

Copy link
Contributor Author

@tejal29 tejal29 Jun 9, 2020

Choose a reason for hiding this comment

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

if we ask users to disable the prompt, they would never see the prompt.
This command would silence it for 2 weeks.

How about we add the command to disable permanently here -> https://skaffold.dev/docs/resources/feedback/ or as part of FAQ.
And change the message to

could not automatically update the survey prompted timestamp - 
please run skaffold config set --survey --global last-prompted 2019-10-20XXX 
or see <> to permanently disable the prompt. 

Or

could not automatically update the survey prompted timestamp
 - please run 
skaffold config set --survey --global last-prompted 2019-10-20XXX 
 or  
run `skaffold config set --survey --global disable-prompt true` to permanently disable the prompt.

Copy link
Contributor

Choose a reason for hiding this comment

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

in any case we're still asking the user to run a command...but after thinking about it a little more I think it's probably fine. we can always change it later if it seems too annoying

@nkubala nkubala merged commit 21a40ec into GoogleContainerTools:master Jun 10, 2020
@tejal29 tejal29 deleted the add_survey_check branch April 15, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wire up ShouldDisplayPrompt to show the survey prompt on every skaffold run.
3 participants