phase2 / rig

Outrigger command line tool
MIT License
11 stars 8 forks source link

Sync bonus round: sync:name, sync:purge, sync:check #165

Closed grayside closed 6 years ago

grayside commented 6 years ago

This set of changes got just tangled enough that I've kept them together. If the time to separate them seems worth doing please say so and I'll take care of it.

Please note this is still a work in progress, and is shared now for context to ongoing unison discussions.

This PR adds the following:

I've also done some light refactoring to cleanup/streamline the sync code.

Fixes #163

tekante commented 6 years ago

Other than the notes I left with some questions and concerning the purge not seeming to work overall I think this is looking pretty good to me.

febbraro commented 6 years ago

@grayside I addresses a few things I saw in my review. I'm approving this, but wanted you to see the changes in case I changed any assumptions you had. Feel free to merge, or approve the additions and I can handle it.

potterme commented 6 years ago

Not a golang expert and not set up to test this, but I like the new commands and the basic logic seems good. Also the pattern for purge cleanup looks good. I have several files right now on my local:

.unison.icons.eot.1e3f49a97047f0a8175c6ea7781ef5da.unison.tmp
.unison.._.DS_Store.f22e592b2ce5945c749e57f21f951e3a.unison.tmp

The DS_Store ones are the most common.

grayside commented 6 years ago

I left the name here as rig project sync:check, do we feel like it should be verify for slightly more semantic context, or doctor for alignment with rig doctor?

febbraro commented 6 years ago

check or verify work for me. I think if we call it doctor we would actually introduce confusion,

Us: "Have you run doctor" Users: "Which one?"

tekante commented 6 years ago

Note that I'm still seeing some issues on the purge command but I don't think they are significant enough to hold up things since they revolve around the unison cleanup.

Test script designed to be run from within the root of the repo after doing a build:

#!/bin/bash

set -e
#VERBOSE=''
VERBOSE=--verbose
DIR=purgetest
mkdir -p $DIR

#./build/darwin/rig --verbose project sync:start --dir=$DIR
#./build/darwin/rig --verbose project sync:stop --dir=$DIR
#./build/darwin/rig --verbose project sync:purge --dir=$DIR

grep version examples/outrigger.example.yml > $DIR/outrigger.yml

pushd $DIR
../build/darwin/rig $VERBOSE project sync:start

touch .unison.icons.eot.1e3f49a97047f0a8175c6ea7781ef5da.unison.tmp
touch .unison.._.DS_Store.f22e592b2ce5945c749e57f21f951e3a.unison.tmp

sleep 2
../build/darwin/rig $VERBOSE project sync:stop
../build/darwin/rig $VERBOSE project sync:purge

First issue I encountered appears to be a duplication of the base directory. Second is an lstat command failing that I haven't tracked down.

Here is diff output that allows for some additional debugging (and avoids the duplicate base directory issue though I haven't fully tested it yet.

diff --git a/commands/project_sync.go b/commands/project_sync.go
index 077ccaa..a81eabb 100644
--- a/commands/project_sync.go
+++ b/commands/project_sync.go
@@ -270,7 +270,7 @@ func (cmd *ProjectSync) RunPurge(ctx *cli.Context) error {
        // Remove sync fragment files.
        cmd.out.Spin("Removing .unison directories")
        if err := util.RemoveFileGlob("*.unison*", workingDir, cmd.out); err != nil {
-               cmd.out.Warning("Could not remove .unison directories")
+               cmd.out.Warning("Could not remove .unison directories: %s", err)
        } else {
                cmd.out.Info("Removed all .unison directories")
        }
diff --git a/util/filesystem.go b/util/filesystem.go
index 3abb5ea..edb5ca6 100644
--- a/util/filesystem.go
+++ b/util/filesystem.go
@@ -16,6 +16,9 @@ func GetExecutableDir() (string, error) {
 // AbsJoin joins the two path segments, ensuring they form an absolute path.
 func AbsJoin(baseDir, suffixPath string) (string, error) {
        absoluteBaseDir, err := filepath.Abs(baseDir)
+       if (baseDir == "" ) {
+               absoluteBaseDir = ""
+       }
        if err != nil {
                return "", fmt.Errorf("Unrecognized working directory: %s: %s", baseDir, err.Error())
        }
@@ -61,6 +64,7 @@ func RemoveFileGlob(glob, targetDirectory string, logger *RigLogger) error {
                                        }

                                        if removeErr := RemoveFile(file, ""); removeErr != nil {
+                                               logger.Verbose("Remove error '%s'", removeErr)
                                                return removeErr
                                        }
                                }
grayside commented 6 years ago

I found and address the double basepath, added some more verbose logging, and added a debug log message where I found the lstat error is throwing: just inside the callback for the filepath.Walk().

To reproduce this error run these commands:

touch .unison
./build/darwin/rig --verbose project sync:purge

This will output the following:

[VERBOSE] Executing: /usr/local/bin/docker ps -aq --filter name=^/rig3-sync ()
[VERBOSE] Executing: /usr/local/bin/docker top  ()
[INFO] No running unison container.
[INFO] Log file does not exist
[VERBOSE] Removing file '/Users/aross/Projects/Outrigger/rig3/.unison'...
[VERBOSE] This is the first err: lstat /Users/aross/Projects/Outrigger/rig3/.unison: no such file or directory
[WARN] Could not remove .unison directories: lstat /Users/aross/Projects/Outrigger/rig3/.unison: no such file or directory
[VERBOSE] Executing: /usr/local/bin/docker volume rm --force rig3-sync ()
[INFO] Sync volume (rig3-sync) removed

Despite the error, the process successfully removes our .unison file.

So what could be going wrong? The file it's complaining about is not a symlink. If I create the file with echo "some content" > .unison the result is the same.

After studying this, my theory is:

In order to fix that, we need to either keep track of the files removed and ignore them when they throw this specific error... or ignore errors on this because honestly it's a nice to have. Working on the latter, just a minute.

febbraro commented 6 years ago

Awesome, thanks for all the hard work @grayside