-
Notifications
You must be signed in to change notification settings - Fork 5.8k
fix: handle recursive watcher ignore paths correctly #13756
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,8 +198,9 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti | |
| eg, ctx := errgroup.WithContext(ctx) | ||
|
|
||
| var ( | ||
| rules []watchRule | ||
| paths []string | ||
| rules []watchRule | ||
| paths []string | ||
| ignoreMatchers []watch.PathMatcher | ||
| ) | ||
| for serviceName, service := range project.Services { | ||
| config, err := loadDevelopmentConfig(service, project) | ||
|
|
@@ -254,6 +255,11 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti | |
| } | ||
| } | ||
| } | ||
| ignore, err := watch.NewDockerPatternMatcher(trigger.Path, trigger.Ignore) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ignoreMatchers = append(ignoreMatchers, ignore) | ||
| paths = append(paths, trigger.Path) | ||
| } | ||
|
|
||
|
|
@@ -268,7 +274,7 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti | |
| return nil, fmt.Errorf("none of the selected services is configured for watch, consider setting a 'develop' section") | ||
| } | ||
|
|
||
| watcher, err := watch.NewWatcher(paths) | ||
| watcher, err := watch.NewWatcher(paths, watch.NewCompositeMatcher(ignoreMatchers...)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] OR-logic composite ignore matcher at watcher layer may suppress directory watching for one trigger due to another trigger's ignore rules All trigger ignore matchers are merged into a single Example: Service A ignores The per-trigger Consider building a per-path/per-trigger watcher or using an intersection (AND) approach — only skip a path if all trigger matchers that cover that path agree it should be ignored. |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,7 +115,7 @@ func (d *fseventNotify) Errors() chan error { | |
| return d.errors | ||
| } | ||
|
|
||
| func newWatcher(paths []string) (Notify, error) { | ||
| func newWatcher(paths []string, _ ...PathMatcher) (Notify, error) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Darwin watcher silently discards the
The fix should store the matcher in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is inconsistent with the naive implementation which use |
||
| dw := &fseventNotify{ | ||
| stream: &fsevents.EventStream{ | ||
| Latency: 50 * time.Millisecond, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,7 @@ type naiveNotify struct { | |||||||||||
| // the notify list. It might be better to store this in a tree | ||||||||||||
| // structure, so we can filter the list quickly. | ||||||||||||
| notifyList map[string]bool | ||||||||||||
| ignore PathMatcher | ||||||||||||
|
|
||||||||||||
| isWatcherRecursive bool | ||||||||||||
| watcher *fsnotify.Watcher | ||||||||||||
|
|
@@ -113,6 +114,10 @@ func (d *naiveNotify) watchRecursively(dir string) error { | |||||||||||
|
|
||||||||||||
| return filepath.WalkDir(dir, func(path string, info fs.DirEntry, err error) error { | ||||||||||||
| if err != nil { | ||||||||||||
| if os.IsPermission(err) && d.shouldIgnore(path) { | ||||||||||||
| logrus.Debugf("Ignoring path: %s", path) | ||||||||||||
| return filepath.SkipDir | ||||||||||||
| } | ||||||||||||
|
ManManavadaria marked this conversation as resolved.
|
||||||||||||
| return err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -180,6 +185,10 @@ func (d *naiveNotify) loop() { //nolint:gocyclo | |||||||||||
| // TODO(dbentley): if there's a delete should we call d.watcher.Remove to prevent leaking? | ||||||||||||
| err := filepath.WalkDir(e.Name, func(path string, info fs.DirEntry, err error) error { | ||||||||||||
| if err != nil { | ||||||||||||
| if os.IsPermission(err) && d.shouldIgnore(path) { | ||||||||||||
| logrus.Debugf("Ignoring path: %s", path) | ||||||||||||
| return filepath.SkipDir | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+188
to
+191
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same this is already catch by block lines 204-206 |
||||||||||||
| return err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -252,12 +261,52 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { | |||||||||||
| // - A child of a directory that's in our notify list, or | ||||||||||||
| // - A parent of a directory that's in our notify list | ||||||||||||
| // (i.e., to cover the "path doesn't exist" case). | ||||||||||||
| // | ||||||||||||
| // We prioritize "parent of watched path" checks before ignore checks so | ||||||||||||
| // one trigger's ignore rules can't hide another trigger's nested watch root. | ||||||||||||
| isChildOfWatchedDir := false | ||||||||||||
| for root := range d.notifyList { | ||||||||||||
| if pathutil.IsChild(root, path) || pathutil.IsChild(path, root) { | ||||||||||||
| if pathutil.IsChild(path, root) { | ||||||||||||
| return false | ||||||||||||
| } | ||||||||||||
| if pathutil.IsChild(root, path) { | ||||||||||||
| isChildOfWatchedDir = true | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // skip the dir if ignore rules match this full subtree. | ||||||||||||
| if d.shouldIgnoreEntireDir(path) { | ||||||||||||
| return true | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return !isChildOfWatchedDir | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| func (d *naiveNotify) shouldIgnoreEntireDir(path string) bool { | ||||||||||||
| if d.ignore == nil { | ||||||||||||
| return false | ||||||||||||
| } | ||||||||||||
| return true | ||||||||||||
| matches, err := d.ignore.MatchesEntireDir(path) | ||||||||||||
| if err != nil { | ||||||||||||
| logrus.Debugf("error checking ignored directory %q: %v", path, err) | ||||||||||||
| return false | ||||||||||||
| } | ||||||||||||
| if matches { | ||||||||||||
| return true | ||||||||||||
| } | ||||||||||||
| return false | ||||||||||||
|
Comment on lines
+294
to
+297
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| func (d *naiveNotify) shouldIgnore(path string) bool { | ||||||||||||
| if d.ignore == nil { | ||||||||||||
| return false | ||||||||||||
| } | ||||||||||||
| matches, err := d.ignore.Matches(path) | ||||||||||||
| if err != nil { | ||||||||||||
| logrus.Debugf("error checking ignored path %q: %v", path, err) | ||||||||||||
| return false | ||||||||||||
| } | ||||||||||||
| return matches | ||||||||||||
|
Comment on lines
+300
to
+309
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function isn't needed, it's only called from the 2 previous unnecessary check blocks above |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| func (d *naiveNotify) add(path string) error { | ||||||||||||
|
|
@@ -270,7 +319,7 @@ func (d *naiveNotify) add(path string) error { | |||||||||||
| return nil | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| func newWatcher(paths []string) (Notify, error) { | ||||||||||||
| func newWatcher(paths []string, ignore PathMatcher) (Notify, error) { | ||||||||||||
| fsw, err := fsnotify.NewWatcher() | ||||||||||||
| if err != nil { | ||||||||||||
| if strings.Contains(err.Error(), "too many open files") && runtime.GOOS == "linux" { | ||||||||||||
|
|
@@ -297,9 +346,9 @@ func newWatcher(paths []string) (Notify, error) { | |||||||||||
| } | ||||||||||||
| notifyList[path] = true | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| wmw := &naiveNotify{ | ||||||||||||
| notifyList: notifyList, | ||||||||||||
| ignore: ignore, | ||||||||||||
| watcher: fsw, | ||||||||||||
| events: fsw.Events, | ||||||||||||
| wrappedEvents: wrappedEvents, | ||||||||||||
|
|
||||||||||||
Uh oh!
There was an error while loading. Please reload this page.