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

fix: correctly set the default value of StatusCheck to nil #7089

Conversation

sberss
Copy link
Contributor

@sberss sberss commented Feb 9, 2022

Description

The statusCheck option in the deploy config section was not working as expected. The value from the skaffold config was never picked up, however it was doing the right thing when --status-check was used on the command line.

This method was checking if the StatusCheck value was defined via the command line, and if so returning the value it was set to. However since the flag was set to incorrectly default to true, that method would always return true if --status-check= was not set on the command line.

This PR sets the default value of StatusCheck to nil, as this lines up with its usage and type BoolOrUndefined

@sberss sberss requested a review from a team as a code owner February 9, 2022 09:17
@sberss sberss requested a review from tejal29 February 9, 2022 09:17
@sberss sberss force-pushed the status-check-correct-default-value branch from a987f6a to 91f3f9a Compare February 9, 2022 09:28
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Feb 9, 2022
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #7089 (db285ca) into main (290280e) will decrease coverage by 1.77%.
The diff coverage is 56.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7089      +/-   ##
==========================================
- Coverage   70.48%   68.71%   -1.78%     
==========================================
  Files         515      558      +43     
  Lines       23150    26116    +2966     
==========================================
+ Hits        16317    17945    +1628     
- Misses       5776     6947    +1171     
- Partials     1057     1224     +167     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 209 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 20ae7f6...db285ca. Read the comment docs.

@sberss sberss force-pushed the status-check-correct-default-value branch from 91f3f9a to db285ca Compare February 9, 2022 10:06
@MarlonGamez
Copy link
Contributor

hey @sbe-genomics , thanks for catching this and creating a fix! Would you mind rebasing this PR? the integration test failure is unrelated and we've merged a fix into main

@sberss sberss force-pushed the status-check-correct-default-value branch from 0b453fe to db285ca Compare February 12, 2022 10:37
@sberss
Copy link
Contributor Author

sberss commented Feb 12, 2022

@MarlonGamez done, thanks!

@MarlonGamez MarlonGamez added the kokoro:force-run forces a kokoro re-run on a PR label Feb 15, 2022
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Feb 15, 2022
@MarlonGamez
Copy link
Contributor

Merging this change as the integration test failure is unrelated/needs to be fixed on our end

FlagAddMethod: "Var",
DefinedOn: []string{"dev", "debug", "deploy", "run", "apply"},
IsEnum: true,
NoOptDefVal: "true",
Copy link
Contributor

@gsquared94 gsquared94 Apr 7, 2022

Choose a reason for hiding this comment

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

This line needs to be reverted. It breaks specifying --status-check without any arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants