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: remove the dev override of the force flag #4513

Merged

Conversation

andrewhertog
Copy link
Contributor

@andrewhertog andrewhertog commented Jul 21, 2020

Fixes: #3798
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

Removes the force flag override for the dev command. Usage of the force flag should be consistent in all commands, and should not default to true.
If users wish to enable the --force flag on deploys, it can be added in skaffold.yaml in the additional flags section that each deployment option provides.

User facing changes (remove if N/A)

For users depending on the --force=true with the dev command, the following changes can be added

deploy:
  helm:
  - flags:
      upgrade: ['--force']
  kubectl:
  - flags:
      apply: ['--force=true']
  kustomize:
  - flags:
      apply: ['--force=true']

@andrewhertog andrewhertog marked this pull request as ready for review July 21, 2020 19:52
@andrewhertog andrewhertog requested a review from a team as a code owner July 21, 2020 19:52
@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #4513 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4513      +/-   ##
==========================================
+ Coverage   72.35%   72.37%   +0.01%     
==========================================
  Files         333      333              
  Lines       12959    12959              
==========================================
+ Hits         9377     9379       +2     
+ Misses       2984     2983       -1     
+ Partials      598      597       -1     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 86.36% <ø> (ø)
...affold/kubernetes/portforward/kubectl_forwarder.go 63.41% <0.00%> (+2.43%) ⬆️

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 ac11549...4c9394d. Read the comment docs.

@nkubala
Copy link
Contributor

nkubala commented Jul 21, 2020

lol, nice timing @andrewhertog - I just started looking into this today while you were creating this PR and left a comment on the original issue before I saw you opened this 😆

can you go have a look at my comment and maybe tell me what you think? i'm inclined to accept this PR, but want to fully understand it first.

#3798 (comment)

@andrewhertog
Copy link
Contributor Author

lol, nice timing @andrewhertog - I just started looking into this today while you were creating this PR and left a comment on the original issue before I saw you opened this 😆

can you go have a look at my comment and maybe tell me what you think? i'm inclined to accept this PR, but want to fully understand it first.

#3798 (comment)

I've followed up with a comment on that thread. Hope it helps. I have an integration test that is failing right now, and will look closer at it tomorrow.

@andrewhertog
Copy link
Contributor Author

andrewhertog commented Jul 22, 2020

@nkubala The Integration tests failed with the following:

TestDebug/buildpacks: util.go:204: Timed out waiting for pods [nodejs] ready in namespace skaffoldbdhdf
--- FAIL: TestDebug (1148.00s)
    --- FAIL: TestDebug/kubectl (409.79s)
    --- FAIL: TestDebug/kustomize (303.52s)
    --- FAIL: TestDebug/buildpacks (434.69s)

When I run INTEGRATION_TEST_ARGS="-run=TestDebug/" make integration-in-kind locally, it passes.

Update: Re-ran integration tests on Travis, and it succeeded. first failure must have been a false failure.

@nkubala
Copy link
Contributor

nkubala commented Jul 22, 2020

thanks @andrewhertog, and everyone else who helped clarify this issue!

@nkubala nkubala merged commit a58e7a0 into GoogleContainerTools:master Jul 22, 2020
@andrewhertog andrewhertog deleted the 3798-default-force-false branch July 22, 2020 18: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.

Consider defaulting 'force' to false with Helm v3
3 participants