Skip to content

Implement a notification based watcher#1439

Merged
dgageot merged 6 commits intoGoogleContainerTools:masterfrom
ltouati:fsnotify
Jan 10, 2019
Merged

Implement a notification based watcher#1439
dgageot merged 6 commits intoGoogleContainerTools:masterfrom
ltouati:fsnotify

Conversation

@ltouati
Copy link
Copy Markdown
Contributor

@ltouati ltouati commented Jan 7, 2019

Hi Team,

This is to support file system notifications instead of reading the files one by ones. It is specially important on MacOS where the current notification scheme is killing the laptop battery.

Let me know your feedback

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 7, 2019 •
edited
Loading

Codecov Report

Merging #1439 into master will decrease coverage by 0.27%.
The diff coverage is 24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
- Coverage   44.53%   44.25%   -0.28%     
==========================================
  Files         112      112              
  Lines        4596     4634      +38     
==========================================
+ Hits         2047     2051       +4     
- Misses       2342     2375      +33     
- Partials      207      208       +1
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/dev.go 0% <0%> (ø) ⬆️
pkg/skaffold/watch/triggers.go 21.12% <20.45%> (-12.21%) ⬇️
pkg/skaffold/watch/watch.go 82.6% <60%> (-3.44%) ⬇️

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 3795677...1c5afbd. Read the comment docs.

dgageot
dgageot suggested changes Jan 7, 2019
View reviewed changes
dgageot
dgageot reviewed Jan 7, 2019
View reviewed changes
@ltouati
Copy link
Copy Markdown
Contributor Author

ltouati commented Jan 8, 2019

Hi @dgageot just implemented the requested changes, let me know if ok for you

@googlebot
Copy link
Copy Markdown

googlebot commented Jan 8, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

r2d4
r2d4 reviewed Jan 8, 2019
View reviewed changes
@r2d4
Copy link
Copy Markdown
Contributor

r2d4 commented Jan 9, 2019

FWIW I tried this out and it seemed to work quite well.

@dgageot
Improve the fsnotify trigger
1c5afbd 
 - Add debouncing
 - Always stop triggers

Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Copy link
Copy Markdown
Contributor

dgageot commented Jan 10, 2019

@ltouati Are you ok with my changes? The CLA requires that we both agree. I do

@r2d4 r2d4 added the kokoro:run runs the kokoro jobs on a PR label Jan 10, 2019
@ltouati
Copy link
Copy Markdown
Contributor Author

ltouati commented Jan 10, 2019

Hi @dgageot

Yes I am ok with your change

@dgageot dgageot merged commit 3402a94 into GoogleContainerTools:master Jan 10, 2019
@ltouati ltouati deleted the fsnotify branch January 10, 2019 18:25
nkubala added a commit to nkubala/skaffold that referenced this pull request Jan 22, 2019
@nkubala
Revert "Merge pull request GoogleContainerTools#1439 from ltouati/fsn…
0a7137b 
…otify"

This reverts commit 3402a94, reversing
changes made to 3795677.
nkubala added a commit to nkubala/skaffold that referenced this pull request Jan 22, 2019
@nkubala
Revert "Merge pull request GoogleContainerTools#1439 from ltouati/fsn…
03605a2 
…otify"

This reverts commit 3402a94, reversing
changes made to 3795677.
nkubala added a commit that referenced this pull request Jan 22, 2019
@nkubala
Merge pull request #1508 from nkubala/revert_fsnotify
037b39a 
Revert "Merge pull request #1439 from ltouati/fsnotify"
dgageot added a commit to dgageot/skaffold that referenced this pull request Jan 29, 2019
@dgageot
Properly stop Triggers
c668066 
This fix was introduced by, but unrelated to, GoogleContainerTools#1439

Signed-off-by: David Gageot <david@gageot.net>
dgageot added a commit to dgageot/skaffold that referenced this pull request Jan 30, 2019
@dgageot
Properly stop Triggers
57641f1 
This fix was introduced by, but unrelated to, GoogleContainerTools#1439

Signed-off-by: David Gageot <david@gageot.net>
balopat pushed a commit that referenced this pull request Jan 30, 2019
@dgageot @balopat
Improve triggers (#1561)
fe6e441 
* interval shouldn’t be exported
* Properly stop Triggers

This fix was introduced by, but unrelated to, #1439

Signed-off-by: David Gageot <david@gageot.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@balopat balopat Awaiting requested review from balopat

@nkubala nkubala Awaiting requested review from nkubala

2 more reviewers

@dgageot dgageot dgageot approved these changes

@r2d4 r2d4 r2d4 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kokoro:run runs the kokoro jobs on a PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ltouati @codecov-io @googlebot @r2d4 @dgageot