Skip to content

add flag --survey to set to set/unset disable survey prompt #3732

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

Merged
merged 3 commits into from
Mar 10, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Feb 26, 2020

Relates to #3011
Shd merge before #3733

In this PR, add a hidden flag --survey to set disable-prompt field in upcoming PRs

Changes:

  1. Use FieldByIndex instead of FieldByName to set, unset values

Testing Done:

mv ~/.skaffold/config ~/.skaffold/config_old

make && ./out/skaffold config set  --survey --global disable-prompt true
set global value disable-prompt to true
tejaldesai@@skaffold (add_survey_set)$ cat ~/.skaffold/config 
global:
  survey:
    disable-prompt: true
kubeContexts: []
tejaldesai@@skaffold (add_survey_set)$ 
 ./out/skaffold config set  --survey  disable-prompt true
set value disable-prompt to true for context gke_tejal-test_us-central1-a_dump
tejaldesai@@skaffold (add_survey_set)$ cat ~/.skaffold/config 
global:
  survey:
    disable-prompt: true
kubeContexts:
- kube-context: gke_tejal-test_us-central1-a_dump
  survey:
    disable-prompt: true
tejaldesai@@skaffold (add_survey_set)$ 
./out/skaffold config unset  --survey  disable-prompt
unset value disable-prompt for context gke_tejal-test_us-central1-a_dump
tejaldesai@@skaffold (add_survey_set)$ cat ~/.skaffold/config 
global:
  survey:
    disable-prompt: true
kubeContexts:
- kube-context: gke_tejal-test_us-central1-a_dump
  survey: {}
./out/skaffold config unset  --survey  --global disable-prompt
unset global value disable-prompt
tejaldesai@@skaffold (add_survey_set)$ cat ~/.skaffold/config 
global:
  survey: {}
kubeContexts:
- kube-context: gke_tejal-test_us-central1-a_dump
  survey: {}
tejaldesai@@skaffold (add_survey_set)$ 


@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #3732 into master will decrease coverage by 0.03%.
The diff coverage is 89.18%.

Impacted Files Coverage Δ
cmd/skaffold/app/cmd/config/flags.go 100% <100%> (ø) ⬆️
cmd/skaffold/app/cmd/config/set.go 83.83% <88.57%> (-0.38%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 65.85% <0%> (-2.44%) ⬇️
pkg/skaffold/deploy/helm.go 77.48% <0%> (-1.17%) ⬇️

@nkubala
Copy link
Contributor

nkubala commented Feb 29, 2020

just trying this out, I think this would be a lot more intuitive if we handled the global config git style, e.g. git config set user.name nick

so for the survey this would be skaffold config set survey.disablePrompt true. how doable do you think that would be?

@tejal29
Copy link
Contributor Author

tejal29 commented Mar 5, 2020

just trying this out, I think this would be a lot more intuitive if we handled the global config git style, e.g. git config set user.name nick

so for the survey this would be skaffold config set survey.disablePrompt true. how doable do you think that would be?

Sorry this got lost in my inbox.
survey.disablePrompt was my first choice but it was a very uninfeasible and messy due to recursive parsing of all ['survey', 'disable-Prompt`] here

I looked into what kubectl does and it has separate commands to set specific structs and went with that approach

kubectl config set --help
 set-cluster     Sets a cluster entry in kubeconfig
  set-context     Sets a context entry in kubeconfig
  set-credentials Sets a user entry in kubeconfig

It supports both,

  # Set only the server field on the e2e cluster entry without touching other values.
  kubectl config set-cluster e2e --server=https://1.2.3.4

kubectl config set clusters.e2e.server https://1.2.3.4

and we can add support for this later.

WDYT?

@tejal29
Copy link
Contributor Author

tejal29 commented Mar 10, 2020

@nkubala can you take a look again after our discussion?

@nkubala
Copy link
Contributor

nkubala commented Mar 10, 2020

@tejal29 sounds good. I opened #3810 to track

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.

this seems fine to me. reflection code is always hard to review 😨

@nkubala nkubala merged commit 181c1a8 into GoogleContainerTools:master Mar 10, 2020
@tejal29 tejal29 deleted the add_survey_set branch April 15, 2021 07:35
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.

3 participants