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 status-check option to the skaffold.yaml #4904

Closed
code-is-art opened this issue Oct 13, 2020 · 7 comments · Fixed by #5706
Closed

Add status-check option to the skaffold.yaml #4904

code-is-art opened this issue Oct 13, 2020 · 7 comments · Fixed by #5706
Assignees
Labels
area/status-check good first issue Good for newcomers help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/feature-request kind/usability priority/p3 agreed that this would be good to have, but no one is available at the moment.
Milestone

Comments

@code-is-art
Copy link

Would like to request that status-check be included in the skaffold.yaml for ease of development. I currently updated to the latest version from a very old version and was confused as to why I had to look at my k8s logs to find out what had happened to my pods until I discovered that the current version of skaffold has status-check set to true by default. I believe this is just fine but now I have to type --status-check=false everytime I want to see a crashed pod log when I am running skaffold dev. It would be great if that flag could be included in the skaffold.yaml so that one can implicitly set it for dev and debug.

  • Skaffold version: 1.15...
  • Operating system: OS X 10.14.6
@MarlonGamez
Copy link
Contributor

I think this is pretty reasonable, and seems easy enough to implement. PRs welcome! :)

@tejal29
Copy link
Contributor

tejal29 commented Oct 26, 2020

making triage-party happy!

@nkubala nkubala added good first issue Good for newcomers help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/feature-request priority/p3 agreed that this would be good to have, but no one is available at the moment. labels Mar 19, 2021
@maggieneterval
Copy link
Contributor

If this issue is still available to be worked on, please feel free to assign me. Thanks!

@nkubala
Copy link
Contributor

nkubala commented Apr 1, 2021

@maggieneterval feel free to take it!

@nkubala nkubala added this to the v1.22.0 milestone Apr 1, 2021
@maggieneterval
Copy link
Contributor

I looked into this a bit and wanted to confirm the intended behavior before submitting a PR.

Existing behavior:

--status-check flag healthcheck enabled?
unset yes
true yes
false no

New behavior:

--status-check flag StatusCheck in skaffold.yaml healthcheck enabled?
unset false no
true false yes
false false no
unset true yes
true true yes
false true no

The existing skaffold healthcheck is enabled by default, and must be explicitly disabled by using the --status-check=false flag. If we add a new StatusCheck field to the skaffold.yaml, does this mean that running skaffold fix should automatically add StatusCheck: true to preserve the existing enabled-by-default behavior? Or is there some way I'm missing to distinguish false from unset in the skaffold.yaml? Also just wanted to confirm since I am new to Skaffold that, for a feature that is intended to be on by default, the complexity of having multiple ways to disable it is justified.

Thank you!

@tejal29
Copy link
Contributor

tejal29 commented Apr 15, 2021

@maggieneterval The above approach looks good to me.

@maggieneterval
Copy link
Contributor

Hi @tejal29, thanks for the reply! I opened #5706.

Upon further investigation, it appears that there is no way to distinguish a true from an unset --status-check flag value (unset defaults to true).

Updated new behavior:

--status-check flag StatusCheck in skaffold.yaml healthcheck enabled?
true (default) true (default) yes
true (default) false no*
false false no
false true (default) no

*The healthcheck will be disabled when the --status-check flag is true (or true by default when unset) and the StatusCheck field is false. If the behavior were to enable the healthcheck in this case, then users would have no way of disabling the healthcheck without explicitly setting --status-check=false (which was the use case for this issue).

maggieneterval added a commit to maggieneterval/skaffold that referenced this issue Apr 23, 2021
maggieneterval added a commit to maggieneterval/skaffold that referenced this issue Apr 28, 2021
maggieneterval added a commit to maggieneterval/skaffold that referenced this issue Apr 30, 2021
tejal29 pushed a commit that referenced this issue Apr 30, 2021
* Add StatusCheck field to skaffold.yaml (#4904)

* Change StatusCheck field from bool to *bool

* Fail if one pipeline has StatusCheck set to true and another has it set to false

* Move new statusCheck field from v2beta15.json to v2beta16.json

* Short circuit the `enabled && disabled` StatusCheck check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/status-check good first issue Good for newcomers help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/feature-request kind/usability priority/p3 agreed that this would be good to have, but no one is available at the moment.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants