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

Re-enable survey prompt #4764

Merged
merged 3 commits into from
Sep 11, 2020
Merged

Re-enable survey prompt #4764

merged 3 commits into from
Sep 11, 2020

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Sep 8, 2020

fixes #4630

also, slightly change wording and add a bit of color.

@nkubala nkubala requested a review from a team as a code owner September 8, 2020 22:40
@nkubala nkubala requested a review from tstromberg September 8, 2020 22:40
Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

Which commands will this be enabled for? I want to make sure that it's only for a specific set of white-listed commands where it makes the most sense to show, rather than "help" and "version".

@tejal29
Copy link
Contributor

tejal29 commented Sep 9, 2020

Which commands will this be enabled for? I want to make sure that it's only for a specific set of white-listed commands where it makes the most sense to show, rather than "help" and "version".

Currently, we are enabling this for all commands.

@nkubala
Copy link
Contributor Author

nkubala commented Sep 9, 2020

Which commands will this be enabled for? I want to make sure that it's only for a specific set of white-listed commands where it makes the most sense to show, rather than "help" and "version".

this is a good point, showing the survey text for help really makes no sense. I think we probably only want to show it for "interactive" subcommands - dev, debug, run, build, deploy, maybe render. whereas it might not make sense on init, and definitely doesn't make sense on credits or version.

i'll update this PR and list out the commands it's enabled for once i do.

@nkubala
Copy link
Contributor Author

nkubala commented Sep 9, 2020

whoops, totally forgot we added this already: #4622

fixing the tests then this should be ready

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #4764 into master will increase coverage by 0.05%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4764      +/-   ##
==========================================
+ Coverage   73.79%   73.84%   +0.05%     
==========================================
  Files         345      347       +2     
  Lines       13744    13753       +9     
==========================================
+ Hits        10142    10156      +14     
+ Misses       2970     2965       -5     
  Partials      632      632              
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/cmd.go 68.34% <0.00%> (ø)
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
pkg/skaffold/schema/v2beta6/upgrade.go 100.00% <ø> (ø)
pkg/skaffold/schema/versions.go 82.75% <ø> (ø)
pkg/skaffold/schema/v2beta7/config.go 100.00% <100.00%> (ø)
pkg/skaffold/schema/v2beta7/upgrade.go 100.00% <100.00%> (ø)
pkg/skaffold/survey/survey.go 52.94% <100.00%> (+7.48%) ⬆️
pkg/skaffold/deploy/helm.go 78.77% <0.00%> (-0.26%) ⬇️
pkg/skaffold/docker/context.go 80.00% <0.00%> (ø)
pkg/skaffold/build/cluster/kaniko.go 0.00% <0.00%> (ø)
... and 2 more

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 5b1f940...f1d416c. Read the comment docs.

pkg/skaffold/survey/survey.go Outdated Show resolved Hide resolved
@nkubala nkubala merged commit c0a7e78 into GoogleContainerTools:master Sep 11, 2020
@nkubala nkubala deleted the survey branch September 11, 2020 18:59
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.

Follow up enabling survey prompt.
5 participants