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

Enable file-watching for debug #4089

Merged
merged 10 commits into from
Jun 19, 2020

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented May 3, 2020

Fixes #2351
Fixes #3272
Fixes #4129

Description

Pulls in the dev-like arguments to control image rebuilding and redeploying such as:

  • --watch-image to restrict the images to be monitored
  • --trigger — for this I kept the same "notify" default, but I wonder if manual would make more sense
  • --auto-build, --auto-deploy, and --auto-sync but defaulting to false.

This at least allows users to enable rebuilding and redeploying on change, which is useful for Go in particular.

User facing changes

skaffold debug now supports listing images to watch and a watch-trigger, though they are essentially non-functional and not worth mentioning since --auto-build, --auto-deploy, and --auto-sync default to false, and are hidden.

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #4089 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4089      +/-   ##
==========================================
- Coverage   71.77%   71.74%   -0.04%     
==========================================
  Files         324      324              
  Lines       12499    12502       +3     
==========================================
- Hits         8971     8969       -2     
- Misses       2958     2965       +7     
+ Partials      570      568       -2     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 86.36% <ø> (ø)
cmd/skaffold/app/cmd/debug.go 100.00% <100.00%> (ø)
cmd/skaffold/app/cmd/dev.go 82.85% <100.00%> (-3.81%) ⬇️
pkg/skaffold/deploy/resource/deployment.go 74.48% <0.00%> (-0.77%) ⬇️
pkg/skaffold/debug/transform_nodejs.go 80.86% <0.00%> (-0.33%) ⬇️
pkg/diag/validator/pod.go 1.28% <0.00%> (-0.07%) ⬇️
pkg/skaffold/debug/transform_jvm.go 94.62% <0.00%> (-0.06%) ⬇️
pkg/skaffold/event/event.go 90.59% <0.00%> (+0.07%) ⬆️
pkg/skaffold/debug/cnb.go 93.10% <0.00%> (+0.51%) ⬆️
pkg/skaffold/debug/transform_go.go 85.85% <0.00%> (+2.02%) ⬆️
... and 1 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 fddc954...58a77f8. Read the comment docs.

@briandealwis
Copy link
Member Author

PTAL @dgageot

@briandealwis
Copy link
Member Author

briandealwis commented May 11, 2020

Yeah, it works because it uses a different temporary variable. When I start skaffold debug in examples/getting-started, and then make a change to main.go, there are no rebuilds.

But you're right — I should make a test here.

Add `dev`-like hidden flags for `--auto-build`, `--auto-deploy`, and
`--auto-sync`, but default to being disabled to avoid redeploying
on file changes.  To workaround spf13/pflag#257,
both `dev` and `debug` use local copies of the flags to ensure their
settings are independent.
@briandealwis
Copy link
Member Author

PTAL @dgageot — added some tests to verify that the arguments are set and are independent of each other. I de-aliased dev too so that NewCmdDebug and NewCmdDev can be called independently of each other.

@briandealwis
Copy link
Member Author

It turns out that our runner creation depends on the implicit initialization of the trigger type from Dev():

TestDeploy: helper.go:194: skaffold deploy: exit status 1, 
    TestDeploy: panic.go:617: creating runner: creating watch trigger: unsupported trigger: 

dgageot
dgageot previously requested changes Jun 8, 2020
cmd/skaffold/app/cmd/debug.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/debug_test.go Outdated Show resolved Hide resolved
cmd/skaffold/app/cmd/debug_test.go Outdated Show resolved Hide resolved
@briandealwis
Copy link
Member Author

Using the common flags is much nicer.

@briandealwis
Copy link
Member Author

PTAL @dgageot

@briandealwis briandealwis dismissed dgageot’s stale review June 19, 2020 03:35

changes addressed

@briandealwis briandealwis requested a review from a team as a code owner June 19, 2020 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants