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

Allow skaffold dev —watch image #925

Merged
merged 3 commits into from
Sep 5, 2018

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Aug 23, 2018

The idea is to be able to run a dev loop that is focused on a few artifacts instead of all the artifacts. For eg, with the micro-services example, I could run skaffold dev --watch-image app to only watch the code for the leeroy-app part.

Signed-off-by: David Gageot david@gageot.net

@dgageot dgageot added the wip label Aug 23, 2018
@dgageot dgageot requested review from balopat and r2d4 as code owners August 23, 2018 06:25
@dgageot dgageot force-pushed the skaffold-dev-filter branch from 8c118c4 to ca9502b Compare August 23, 2018 06:35
@codecov-io
Copy link

codecov-io commented Aug 23, 2018

Codecov Report

Merging #925 into master will increase coverage by 0.1%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #925     +/-   ##
=========================================
+ Coverage   43.28%   43.38%   +0.1%     
=========================================
  Files          63       63             
  Lines        2629     2639     +10     
=========================================
+ Hits         1138     1145      +7     
- Misses       1372     1374      +2     
- Partials      119      120      +1
Impacted Files Coverage Δ
pkg/skaffold/config/options.go 81.81% <ø> (ø) ⬆️
cmd/skaffold/app/cmd/cmd.go 0% <0%> (ø) ⬆️
pkg/skaffold/runner/runner.go 56.21% <77.77%> (+1.1%) ⬆️

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 3cfdf89...1ac9d35. Read the comment docs.

@dgageot dgageot force-pushed the skaffold-dev-filter branch from ca9502b to 0e3bf24 Compare August 23, 2018 11:18
@dgageot dgageot changed the title Allow skaffold dev —filter image Allow skaffold dev —watch image Aug 23, 2018
@dgageot dgageot force-pushed the skaffold-dev-filter branch from 0e3bf24 to 992f552 Compare August 24, 2018 08:12
@ahmetb
Copy link
Contributor

ahmetb commented Aug 29, 2018

I’d recommend calling this —watch-image/-w to be verbose. Almost everyone has to read the help description to figure out how to use it otherwise.

@dgageot
Copy link
Contributor Author

dgageot commented Aug 29, 2018

Thanks @ahmetb. I think I'll also add support for regexps

@ahmetb
Copy link
Contributor

ahmetb commented Aug 29, 2018

What’s the use case do you have in mind with regexps? I suspect directories might work better than image names if multiple images (that can be selected via a regexp) are involved.

@dgageot
Copy link
Contributor Author

dgageot commented Aug 29, 2018

I don't really have one in mind :-) In fact I tend to dislike regexps. You just convinced me to not bother and see if there's a need for it.

@dgageot dgageot force-pushed the skaffold-dev-filter branch from 992f552 to f625378 Compare August 29, 2018 17:12
@dgageot dgageot removed the wip label Aug 29, 2018
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

Nits + would like to have a test please!

@@ -128,6 +128,7 @@ func setFlagsFromEnvVariables(commands []*cobra.Command) {

func AddDevFlags(cmd *cobra.Command) {
cmd.Flags().BoolVar(&opts.Cleanup, "cleanup", true, "Delete deployments after dev mode is interrupted")
cmd.Flags().StringArrayVarP(&opts.Watch, "watch-image", "w", nil, "Choose which artifacts to watch. Matches against image names. Default is to watch sources for all artifacts.")
Copy link
Contributor

Choose a reason for hiding this comment

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

"Choose which artifacts to watch. Artifacts with image names that contain the expression will be watched only. Default is to watch sources for all artifacts."

return true
}

for _, name := range r.opts.Watch {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: name -> watchExpression

@ahmetb
Copy link
Contributor

ahmetb commented Aug 29, 2018

I don't really have one in mind :-) In fact I tend to dislike regexps. You just convinced me to not bother and see if there's a need for it.

Yeah I think there are two ways to go here. Either accept --watch-dir[] or --watch-image[]. I think --watch-image is good and I would not mind listing multiple images at once.

Something like "interactive skaffold" came up in another issue (basically ability to stop building/watching certain images on the fly without turning skaffold off) but that's off-topic probably for now.

Signed-off-by: David Gageot <david@gageot.net>
@dgageot dgageot force-pushed the skaffold-dev-filter branch from f625378 to dd01d2a Compare August 31, 2018 05:12
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Copy link
Contributor Author

dgageot commented Aug 31, 2018

ping @balopat

@dgageot dgageot merged commit e78bf17 into GoogleContainerTools:master Sep 5, 2018
@dgageot dgageot deleted the skaffold-dev-filter branch December 28, 2018 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants