add a --watch flag to oc rsync by bparees · Pull Request #8268 · openshift/origin

bparees · 2016-03-28T19:11:49Z

add a --watch flag to oc rsync so it watches the filesystem for changes and automatically syncs when the filesystem changes

bparees · 2016-03-28T19:26:04Z

@csrwng @smarterclayton ptal

bparees · 2016-03-28T19:29:16Z

also if one of you could test this on a mac that'd be awesome :)

csrwng · 2016-03-28T20:46:10Z

So it works on the Mac, at least on my simple test. One thing I ran into though as soon as I tried it is that fsnotify complained of not being able to open a file. I realized that the default limit for open files on the Mac is tiny (256). We may want to mention that in the doc.

bparees · 2016-03-28T20:47:56Z

yeah so as you see i have a comment in the code about that, it's probably so common that the error should say "try this...." if we get an error adding the watch.

smarterclayton · 2016-03-28T20:55:53Z

pkg/cmd/cli/cmd/rsync/rsync.go

+}
+
+// recursive logic from https://github.com/bronze1man/kmg/blob/master/fsnotify/Watcher.go
+func watchRecursion(watcher *fsnotify.Watcher, path string) error {


Feels like this should be in a different package

yeah, i debated moving this whole watcher mess to a helper package. i can do that.

bparees · 2016-03-30T21:34:37Z

@smarterclayton helpers moved to package
@rhcarvalho race condition fixed, watchError returned

mfojtik · 2016-03-31T11:15:16Z

@bparees lgtm. as @csrwng we might run into problems when you start watching too many files (even on linux)... we should probably document what users should do when this problem happens.

mfojtik · 2016-03-31T11:15:50Z

@bparees test will come as follow up, right? ;-)

rhcarvalho · 2016-03-31T14:21:28Z

pkg/cmd/cli/cmd/rsync/rsync.go

+		time.Sleep(2 * time.Second)
+	}
+	// unreachable
+	return nil


The two lines above can/should be removed. The compiler knows about unreachable code, and so does go vet:

$ go vet pkg/cmd/cli/cmd/rsync/rsync.go pkg/cmd/cli/cmd/rsync/rsync.go:308: unreachable code exit status 1

rhcarvalho · 2016-03-31T14:34:05Z

This might be a starting point for documenting the behavior (incl. max files):

https://github.com/fsnotify/fsnotify/wiki/FAQ

rhcarvalho · 2016-03-31T14:39:08Z

pkg/cmd/cli/cmd/rsync/rsync.go

+				if event.Op&fsnotify.Remove == fsnotify.Remove {
+					watcher.Remove(event.Name)
+				} else {
+					fsnotification.AddRecursiveWatch(watcher, event.Name)


AFAIK we don't need to add watchers per file, only directories. Watching a directory gets events for all files.

howeyc/fsnotify#56 (comment) for reference.

Oh well, AddRecursiveWatch actually ignores paths that are not directories. That's subtle.

Shouldn't we check the error returned by fsnotification.AddRecursiveWatch?

will add a comment to the function.

and yes, should. will assign it to watchError

rhcarvalho · 2016-04-06T19:30:38Z

pkg/util/fsnotification/fsnotification.go

+	return nil
+}
+
+// getSubFolders recursively retrieves all subfolders of the specified path


Missing a final period.

rhcarvalho · 2016-04-06T19:33:26Z

Two documentation nits, code LGTM. Nice stuff @bparees, thanks!

bparees · 2016-04-06T20:36:56Z

nits addressed, thanks @rhcarvalho

smarterclayton · 2016-04-26T16:07:49Z

pkg/cmd/cli/cmd/rsync/rsync.go

 	cmd.Flags().StringVar(&o.RsyncInclude, "include", "", "rsync - include files matching specified pattern")
 	cmd.Flags().BoolVar(&o.RsyncProgress, "progress", false, "rsync - show progress during transfer")
 	cmd.Flags().BoolVar(&o.RsyncNoPerms, "no-perms", false, "rsync - do not transfer permissions")
+	cmd.Flags().BoolVarP(&o.Watch, "watch", "w", false, "Watch directory for changes and resync automatically")


This is consistent with get, correct? Will we ever introduce a "watch" flag?

if by "consistent" with get you mean that get has a --watch flag, then yes. Whether what get --watch does is similar enough to rsync --watch that they should be using the same argument name is debatable. (get is reading something and display you the result whenever it changes. rsync is monitoring the filesystem for changes and sending them to the container whenever it changes). So...similarish, not identical.

would we ever want the equivalent of get --watch functionality in rsync? I have a hard time envisioning that we would.

overall watch feels like the right name here i think.

would we ever want the equivalent of get --watch functionality in rsync? I have a hard time envisioning that we would.

by which i mean, i have a hard time envisioning what that would mean/do.

It's fine, just wanted to check

On Tue, Apr 26, 2016 at 12:42 PM, Ben Parees notifications@github.com
wrote:

In pkg/cmd/cli/cmd/rsync/rsync.go
#8268 (comment):

@@ -111,6 +117,7 @@ func NewCmdRsync(name, parent string, f *clientcmd.Factory, out, errOut io.Write
cmd.Flags().StringVar(&o.RsyncInclude, "include", "", "rsync - include files matching specified pattern")
cmd.Flags().BoolVar(&o.RsyncProgress, "progress", false, "rsync - show progress during transfer")
cmd.Flags().BoolVar(&o.RsyncNoPerms, "no-perms", false, "rsync - do not transfer permissions")

cmd.Flags().BoolVarP(&o.Watch, "watch", "w", false, "Watch directory for changes and resync automatically")

would we ever want the equivalent of get --watch functionality in rsync? I
have a hard time envisioning that we would.

by which i mean, i have a hard time envisioning what that would mean/do.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8268/files/b9f4b16543df4fb40bdd86f2797f6d2dd2e14093..5b20fcd4b5c6e42bdf67cf7b118d57d95e532e6f#r61120407

smarterclayton · 2016-04-26T23:14:04Z

So much pull through, so much fail. [test]

On Tue, Apr 26, 2016 at 7:05 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3289/)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8268 (comment)

…cb19053

openshift-bot · 2016-04-28T13:35:25Z

Evaluated for origin test up to 899f157

openshift-bot · 2016-04-28T14:19:06Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3397/)

openshift-bot · 2016-04-28T20:10:29Z

Evaluated for origin testonlyextended up to 899f157

openshift-bot · 2016-04-28T21:42:09Z

continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/119/) (Extended Tests: core(rsync))

bparees · 2016-04-28T22:58:41Z

[merge]

openshift-bot · 2016-04-28T23:00:40Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5740/) (Image: devenv-rhel7_4057)

openshift-bot · 2016-04-28T23:00:40Z

Evaluated for origin merge up to 899f157

bparees · 2016-04-29T00:55:46Z

@thesteve0 fyi. merged.

thesteve0 · 2016-04-29T02:19:05Z

And there was much rejoicing!!!! So this will be in the next origin release?

On Thu, Apr 28, 2016 at 5:55 PM, Ben Parees notifications@github.com
wrote:

@thesteve0 https://github.com/thesteve0 fyi. merged.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8268 (comment)

bparees · 2016-04-29T02:22:28Z

@thesteve0 yeah.

bparees force-pushed the fsnotify branch from b8eaa2c to c882a61 Compare March 28, 2016 19:25

bparees force-pushed the fsnotify branch 2 times, most recently from 8417f21 to dcc7d3c Compare March 28, 2016 19:28

bparees changed the title ~~add a --watch flag to oc rsync so it watches the filesystem for changes and automatically syncs when the filesystem changes~~ add a --watch flag to oc rsync Mar 28, 2016

smarterclayton reviewed Mar 28, 2016
View reviewed changes

bparees force-pushed the fsnotify branch 3 times, most recently from 6e2798c to c0485dc Compare March 30, 2016 19:49

rhcarvalho reviewed Mar 31, 2016
View reviewed changes

rhcarvalho reviewed Apr 6, 2016
View reviewed changes

bparees force-pushed the fsnotify branch 2 times, most recently from fe639f9 to 42faf99 Compare April 6, 2016 20:36

bparees force-pushed the fsnotify branch from 42faf99 to 5b20fcd Compare April 8, 2016 14:23

bparees mentioned this pull request Apr 8, 2016

document rsync --watch openshift/openshift-docs#1880

Merged

smarterclayton reviewed Apr 26, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2016

smarterclayton added the api-approved label Apr 26, 2016

bparees force-pushed the fsnotify branch 2 times, most recently from da64699 to 62808bc Compare April 26, 2016 21:10

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2016

bparees added 2 commits April 27, 2016 08:23

bump(github.com/fsnotify/fsnotify): 3c39c22b2c7b0516d5f2553f1608e5d13…

d5c0e74

…cb19053

introduce --watch capability for oc rsync

899f157

bparees force-pushed the fsnotify branch from 62808bc to 899f157 Compare April 27, 2016 12:24

openshift-bot merged commit 20709a6 into openshift:master Apr 28, 2016

bparees deleted the fsnotify branch April 29, 2016 16:43

ingvagabund mentioned this pull request Sep 2, 2022

pkg/cli/rsync/fsnotification: fix dropped error openshift/oc#1214

Closed

Conversation

bparees commented Mar 28, 2016

Uh oh!

bparees commented Mar 28, 2016

Uh oh!

bparees commented Mar 28, 2016

Uh oh!

csrwng commented Mar 28, 2016

Uh oh!

bparees commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Mar 30, 2016

Uh oh!

mfojtik commented Mar 31, 2016

Uh oh!

mfojtik commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho commented Apr 6, 2016

Uh oh!

bparees commented Apr 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Apr 26, 2016

Uh oh!

openshift-bot commented Apr 28, 2016

Uh oh!

openshift-bot commented Apr 28, 2016

Uh oh!

openshift-bot commented Apr 28, 2016

Uh oh!

openshift-bot commented Apr 28, 2016

Uh oh!

bparees commented Apr 28, 2016

Uh oh!

openshift-bot commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Apr 28, 2016

Uh oh!

bparees commented Apr 29, 2016

Uh oh!

thesteve0 commented Apr 29, 2016

Uh oh!

bparees commented Apr 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

openshift-bot commented Apr 28, 2016 •

edited

Loading