project sync executed in project root directory #86

grayside · 2017-10-14T22:54:56Z

Corrects the assumption in project_sync.go that commands should be executed from the current working directory. Instead, commands are executed the same as rig project scripts -- from the directory in which the configuration file is found.

If no project config is found, using current working directory as the local side of the file sync, per existing shipped behavior.
RemoveLogFile resolution: inline the logic or make a stand-alone API function.

Fixes #67

grayside · 2017-10-15T20:20:43Z

This needs more testing for execution when there isn't any project configuration.

grayside · 2017-10-17T03:10:42Z

The change as implemented removes support for the current working directory as a fallback to their being no configuration files available, so that needs to be put back.

febbraro · 2017-10-18T01:57:34Z

So you are saying that before this PR the root for the sync dir was whatever dir you were in when you ran the command, and not the directory that the outrigger.yml file is in?

Now with this PR it will only work if there is an outrigger.yml and use that file's directory, but we also need to account for the current working directory if that project config file doesn't exist?

Also, with the issue you linked, recursive project config discovery, that seems kind of problematic. Maybe the project config file need to specify the sync dir, and if there is no project config in the recursive heirarchy that specifies a sync dir then it just uses the current dir?

grayside · 2017-10-18T02:45:08Z

So you are saying that before this PR the root for the sync dir was whatever dir you were in when you ran the command, and not the directory that the outrigger.yml file is in?

Correct, if you look at the code, it used os.Getwd() pretty ubiquitously.

Now with this PR it will only work if there is an outrigger.yml and use that file's directory, but we also need to account for the current working directory if that project config file doesn't exist?

Yes, that's what my latest comment discussed. I over-corrected. We wanted to be able to use this functionality outside a project context. That seems like an edge case (similar to us using docker run instead of docker-compose run), but still probably worth supporting for quick experiments.

Also, with the issue you linked, recursive project config discovery, that seems kind of problematic. Maybe the project config file need to specify the sync dir, and if there is no project config in the recursive heirarchy that specifies a sync dir then it just uses the current dir?

The idea is that if an outrigger.yml is found, the rig project sync should be rooted in that directory. If not, it should be the current working directory. It makes sense to me that we further enhance the outrigger.yml unison config to specify a relative path to sync from below the project config structure. Currently all rig project scripts execute in the directory of the outrigger.yml file, we assume it is always at the repo root.

febbraro · 2017-10-19T01:10:00Z

But it is possibly to have a "global" outrigger.yml, say in your $HOME dir, and it is possible without a project config (for various use cases) that it will share your $HOME dir via unison. Should we additionally add a flag that says use $CWD?

grayside · 2017-10-19T05:08:42Z

I think it makes sense to add a flag to designate the localhost side of the sync as a path. So --sync-from=. or --sync-from=$PWD would end up as the current directory, but other options are available, such as if a particular project is structured such that only the contents of a subdirectory should be synced.

In priority order, this gives us:

--sync-from flag
outrigger.yml
Current working directory

As long as we're talking about these options, it's worth noting that we currently have an argument that causes problems for other commands to interact with the sync container, so we should be careful to keep those use cases in mind. #85 for discussion on that.

febbraro · 2017-10-19T12:43:09Z

Would --sync-path or --sync-root work better?

grayside · 2017-10-19T18:16:59Z

Re-introduced the CWD fallback, added --sync-path override, added verbose logging of what the working directory is for sync execution, followed that through to do similar in project scripts and tweaked a couple log messages for easier reading flow.

febbraro · 2017-10-20T13:56:00Z

cli/commands/project_sync.go

+	// process is still watching the "current directory" and so needs the working
+	// directory to be explicitly set.
+	command.Dir = workingDir
+	cmd.out.Verbose.Printf("Script execution - Working Directory: %s", workingDir)


Log message appears wrong. This should be Sync or something besides Script execution

grayside · 2017-10-20T16:54:30Z

Log message appears wrong. Script execution -> Sync execution

febbraro · 2017-10-20T22:13:59Z

cli/commands/project_sync.go

 	cmd.out.Info.Print("Waiting for initial sync detection")

-	var tempFile = fmt.Sprintf(".%s.tmp", logFile)
+	var tempFile = filepath.Join(workingDir, fmt.Sprintf(".test-sync-start.tmp"))


Don;t need a Sprintf here is the log file name is not patterned from a variable. %s or whatevs.

I think what I meant to say is, why are you not using logFile

Also, logFile should already be a full absolute path, so I'm thinking that the filepath.Join is not needed.

This logic predates the PR. I think logFile is used by unison, and we're checking that tempFile is synced. I wonder what happens if a project ignores *.tmp files... might need a parameter or less fraught ext.

febbraro · 2017-10-20T22:16:17Z

cli/commands/project_sync.go

-	volumeName := cmd.GetVolumeName(ctx, cmd.Config)
+	// Determine the working directory for CWD-sensitive operations.
+	var workingDir string
+	if syncPath, err := cmd.DeriveLocalSyncPath(cmd.Config, ctx.String("sync-path")); err != nil {


could you not use workingDir here as it is already defined as a var and avoid the else all together?

I declare the var so it's in function instead of block scope, but the := operator is caught by the compiler as an overwrite. I could nest the whole function inside the if with reversed err check.

febbraro · 2017-10-20T22:19:01Z

cli/commands/project_sync.go

-	cmd.out.Verbose.Printf("Stopping sync with volume: %s", volumeName)
+	// Determine the working directory for CWD-sensitive operations.
+	var workingDir string
+	if syncPath, err := cmd.DeriveLocalSyncPath(cmd.Config, ctx.String("sync-path")); err != nil {


Same, can we use workingDir here and remove the else

febbraro · 2017-10-20T22:25:48Z

cli/commands/project_sync.go

+// Remove the Unison logfile.
+func (cmd *ProjectSync) RemoveLogFile(logFile string, workingDir string) error {
+	command := exec.Command("rm", "-f", logFile)
+	command.Dir = workingDir


If logFile is a variable with workingDir already joined, you dont need to pass in workingDir and set it the command.Dir

My reasoning here is that if it is worth having this broken out as a separate function, and an exported one at that, it makes sense to not make assumptions about the logFile path. In fact, you could argue that this should be checking the logFile path exists.

I don't have strong feelings about folding it back in or helping it to stand alone better. Do you have a preference?

I think we can keep it, I did not realize you intended to make that funciton for public consumption. If so, then don't call it RemoveLogFile just call it RemoveFile and rename the variable and then things make more sense as a general purpose function.

febbraro

Left a few comments about paths and such

grayside · 2017-10-21T04:21:35Z

Improved code flow around workingDir by nesting login inside if blocks.

grayside · 2017-10-23T19:26:00Z

cli/commands/project_sync.go

@@ -38,14 +41,14 @@ func (cmd *ProjectSync) Commands() []cli.Command {
 	start := cli.Command{
 		Name:        "sync:start",
 		Aliases:     []string{"sync"},
-		Usage:       "Start a unison sync on local project directory. Optionally provide a volume name.",
+		Usage:       "Start a Unison sync on local project directory. Optionally provide a volume name.",


Did some renaming so Unison is capitalized in messages.

grayside · 2017-10-23T19:26:33Z

cli/commands/project_sync.go

 	var logFile = fmt.Sprintf("%s.log", volumeName)
-	exec.Command("rm", "-f", logFile).Run()


Created util/filesystem.go for these basic kinds of operations.

grayside · 2017-10-23T19:27:40Z

cli/commands/project_sync.go

+	// The use of os.Stat below is not subject to our working directory configuration,
+	// so to ensure we can stat the log file we convert it to an absolute path.
+	if logFilePath, err := util.AbsJoin(workingDir, logFile); err != nil {
+		cmd.out.Info.Print(err.Error())


I've attempted to fix the inconsistency between absolute paths vs. working direct & relative paths throughout this file, but os.Stat is an edge case that needs support.

grayside · 2017-10-23T19:28:10Z

cli/commands/project_sync.go

+		var timeoutLoopSleep = time.Duration(100) * time.Millisecond
+		// * 10 here because we loop once every 100 ms and we want to get to seconds
+		var timeoutLoops = timeoutSeconds * 10
+		var statSleep = time.Duration(syncWaitSeconds) * time.Second


Moved statSleep up because we were previously looping over an otherwise unchanging operation.

grayside · 2017-10-23T19:29:44Z

cli/commands/project_sync.go

+		cmd.out.Info.Print(err.Error())
+	} else {
+		// Create a temp file to cause a sync action
+		var tempFile = ".rig-check-sync-start"


Renamed this further to remove .tmp suffix and introduce rig in the name to clarify what it is and avoid potential sync ignores around not syncing tempfiles.

grayside · 2017-10-23T19:30:02Z

cli/util/filesystem.go

@@ -0,0 +1,53 @@
+package util


All new code in this file.

febbraro · 2017-10-24T02:17:47Z

So, it will choose either the sync-path provided, or the location of the config file, or the current working directory. Do we think that it makes sense to have a generally configurable path in the sync section of the config file? or do you think that is just generally captured under the --sync=path option and you'd encod ethat directly into your project scripts?

I will do a bit of a deeper review tonight, but i'm thinking this thing is just about good to merge, thanks for all the hard work on it.

grayside · 2017-10-24T05:14:11Z

@febbraro my thinking is that projects will always sync relative to the outrigger.yml, because in that way if you connect via the CLI container all the pieces are the same, which I find tremendously valuable for general intuition. I could see the value of a sync-path option in outrigger.yml, but given the ignorenot modifier for Unison we theoretically support it already. https://www.cis.upenn.edu/~bcpierce/unison/download/releases/stable/unison-manual.html#ignore

grayside · 2017-10-24T15:52:07Z

Travis failed the branch on a partial refactoring I had in the util directory, to take the easy way out I added those changes to this PR.

I've broken up util.go into user_input.go and docker.go and folded image.go into docker.go. When there's more than one file I think semantically naming and categorizing everything makes it easier to find.

…t error.

grayside · 2017-10-25T16:46:08Z

This one wins the award for hardest easy PR I've landed in rig.

project sync executed in project root directory.

e6e0cce

grayside requested review from febbraro, tekante and mike-potter October 14, 2017 22:55

grayside mentioned this pull request Oct 17, 2017

Recursively find outrigger.yml #89

Merged

grayside changed the title ~~project sync executed in project root directory.~~ project sync executed in project root directory [WIP] Oct 18, 2017

Merge branch 'develop' into project-sync-from-root

cd4d0ec

Adam Ross added 2 commits October 19, 2017 11:14

Introduce sync-path override flag and reintroduce CWD fallback

fe84ed8

Verbose logging improvements - show working directory.

94672bb

grayside changed the title ~~project sync executed in project root directory [WIP]~~ project sync executed in project root directory Oct 19, 2017

febbraro reviewed Oct 20, 2017

View reviewed changes

Tweak logging language: script execution -> sync execution.

3ad60c7

febbraro reviewed Oct 20, 2017

View reviewed changes

febbraro requested changes Oct 20, 2017

View reviewed changes

Code flow improvements.

c14a8c3

Adam Ross added 2 commits October 23, 2017 12:20

Capitalize Unison is command help and log messages.

bd15f81

Factor filesystem operations to new util file.

50a3606

go fmt cleanups.

78d1c7e

grayside commented Oct 23, 2017

View reviewed changes

cli/util/filesystem.go

@@ -0,0 +1,53 @@

package util

Copy link

Contributor Author

grayside Oct 23, 2017

Choose a reason for hiding this comment

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

All new code in this file.

Re-arranging util package files.

7360ad9

Adam Ross added 4 commits October 24, 2017 11:29

If unison logfile pre-initialization cannot remove, log as verbose no…

b717af7

…t error.

Add ProjectConfig.NotEmpty function.

7eacda8

Missed logFile => logFilePath rename.

1f2dbe3

Rename --sync-path to --dir

7e7c9f4

febbraro approved these changes Oct 25, 2017

View reviewed changes

febbraro merged commit 452e71a into develop Oct 25, 2017

febbraro deleted the project-sync-from-root branch October 25, 2017 16:00

grayside added this to the v1.4 milestone Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

project sync executed in project root directory #86

project sync executed in project root directory #86

grayside commented Oct 14, 2017 •

edited

grayside commented Oct 15, 2017

grayside commented Oct 17, 2017

febbraro commented Oct 18, 2017

grayside commented Oct 18, 2017

febbraro commented Oct 19, 2017

grayside commented Oct 19, 2017

febbraro commented Oct 19, 2017

grayside commented Oct 19, 2017

febbraro Oct 20, 2017

grayside commented Oct 20, 2017

febbraro Oct 20, 2017

febbraro Oct 20, 2017

febbraro Oct 20, 2017

grayside Oct 21, 2017

febbraro Oct 20, 2017

grayside Oct 21, 2017

febbraro Oct 20, 2017

febbraro Oct 20, 2017

grayside Oct 21, 2017

febbraro Oct 22, 2017

febbraro left a comment

grayside commented Oct 21, 2017

grayside Oct 23, 2017

grayside Oct 23, 2017

grayside Oct 23, 2017

grayside Oct 23, 2017

grayside Oct 23, 2017

grayside Oct 23, 2017

febbraro commented Oct 24, 2017

grayside commented Oct 24, 2017

grayside commented Oct 24, 2017

grayside commented Oct 25, 2017

		var logFile = fmt.Sprintf("%s.log", volumeName)
		exec.Command("rm", "-f", logFile).Run()

project sync executed in project root directory #86

project sync executed in project root directory #86

Conversation

grayside commented Oct 14, 2017 • edited

grayside commented Oct 15, 2017

grayside commented Oct 17, 2017

febbraro commented Oct 18, 2017

grayside commented Oct 18, 2017

febbraro commented Oct 19, 2017

grayside commented Oct 19, 2017

febbraro commented Oct 19, 2017

grayside commented Oct 19, 2017

Choose a reason for hiding this comment

grayside commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

febbraro left a comment

Choose a reason for hiding this comment

grayside commented Oct 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

febbraro commented Oct 24, 2017

grayside commented Oct 24, 2017

grayside commented Oct 24, 2017

grayside commented Oct 25, 2017

grayside commented Oct 14, 2017 •

edited