#461: pull out some logic from request transfer for defaulting so we …

…can use it in both
dotmesh-io · Jul 12, 2018 · f93a109 · f93a109
1 parent 8ab9c50
commit f93a109
Show file tree

Hide file tree

Showing 2 changed files with 99 additions and 88 deletions.
diff --git a/cmd/dm/pkg/commands/s3.go b/cmd/dm/pkg/commands/s3.go
@@ -83,7 +83,7 @@ func NewCmdS3(out io.Writer) *cobra.Command {
 	})
 	subCommand := &cobra.Command{
 		Use:   "clone-subset <remote> <bucket> <prefixes> [--local-name=<dot>]",
-		Short: "Clone an s3 bucket, but only select a subset as dictated by prefixes. (for full bucket clones see dm clone as normal)",
+		Short: "Clone an s3 bucket, but only select a subset as dictated by comma-separated prefixes. (for full bucket clones see dm clone as normal)",
 		// TODO add this to the docs
 		Long: "Online help: https://docs.dotmesh.com/references/cli/#add-a-new-remote-dm-remote-add-name-user-hostname",
 
@@ -94,12 +94,16 @@ func NewCmdS3(out io.Writer) *cobra.Command {
 				if err != nil {
 					return err
 				}
+				peer, filesystemName, branchName, err := resolveTransferArgs(args[:len(args)-1])
+				if err != nil {
+					return err
+				}
 				transferId, err := dm.RequestS3SubsetTransfer(
-					args[0],
-					"pull",
-					cloneLocalVolume,
-					args[2],
+					"pull", peer,
+					localVolumeName, branchName,
+					filesystemName, branchName,
 					prefixes,
+					// TODO also switch to the remote?
 				)
 				if err != nil {
 					return err

diff --git a/cmd/dm/pkg/remotes/api.go b/cmd/dm/pkg/remotes/api.go
@@ -927,83 +927,10 @@ func (dm *DotmeshAPI) RequestTransfer(
 
 	// Let's replace any missing things with defaults.
 	// The defaults depend on whether we're pushing or pulling.
-
-	if direction == "push" {
-		// We are pushing, so if no local filesystem/branch is
-		// specified, take the current one.
-		if localFilesystemName == "" {
-			localFilesystemName, err = dm.Configuration.CurrentVolume()
-			if err != nil {
-				return "", err
-			}
-		}
-
-		if localBranchName == "" {
-			localBranchName, err = dm.Configuration.CurrentBranch()
-			if err != nil {
-				return "", err
-			}
-		}
-	} else if direction == "pull" {
-		// We are pulling, so if no local filesystem/branch is
-		// specified, we take the remote name but strip it of its
-		// namespace. So if we pull "bob/apples", we pull into "apples",
-		// which is really "admin/apples".
-		if localFilesystemName == "" && remoteFilesystemName != "" {
-			_, localFilesystemName, err = ParseNamespacedVolume(remoteFilesystemName)
-			if err != nil {
-				return "", err
-			}
-		}
-	}
-
-	// Split the local volume name's namespace out
-	localNamespace, localVolume, err := ParseNamespacedVolume(localFilesystemName)
+	localNamespace, localVolume, localBranchName, remoteNamespace, remoteVolume, remoteBranchName, err := dm.parseVolumeDetails(remote, peer, direction, localFilesystemName, localBranchName, remoteFilesystemName, remoteBranchName)
 	if err != nil {
 		return "", err
 	}
-
-	// Guess defaults for the remote filesystem
-	var remoteNamespace, remoteVolume string
-	if remoteFilesystemName == "" {
-		// No remote specified. Do we already have a default configured?
-		defaultRemoteNamespace, defaultRemoteVolume, ok := dm.Configuration.DefaultRemoteVolumeFor(peer, localNamespace, localVolume)
-		if ok {
-			// If so, use it
-			remoteNamespace = defaultRemoteNamespace
-			remoteVolume = defaultRemoteVolume
-		} else {
-			// If not, default to the un-namespaced local filesystem name.
-			// This causes it to default into the user's own namespace
-			// when we parse the name, too.
-			remoteNamespace = remote.DefaultNamespace()
-			remoteVolume = localVolume
-		}
-	} else {
-		// Default namespace for remote volume is the username on this remote
-		remoteNamespace, remoteVolume, err = ParseNamespacedVolumeWithDefault(remoteFilesystemName, remote.DefaultNamespace())
-		if err != nil {
-			return "", err
-		}
-	}
-
-	// Remember default remote if there isn't already one
-	_, _, ok := dm.Configuration.DefaultRemoteVolumeFor(peer, localNamespace, localVolume)
-	if !ok {
-		dm.Configuration.SetDefaultRemoteVolumeFor(peer, localNamespace, localVolume, remoteNamespace, remoteVolume)
-	}
-
-	if remoteBranchName == "" {
-		remoteBranchName = localBranchName
-	}
-
-	if remoteBranchName != "" && remoteVolume == "" {
-		return "", fmt.Errorf(
-			"It's dubious to specify a remote branch name " +
-				"without specifying a remote filesystem name.",
-		)
-	}
-
 	if direction == "push" {
 		fmt.Printf("Pushing %s/%s to %s:%s/%s\n",
 			localNamespace, localVolume,
@@ -1088,18 +1015,94 @@ func (dm *DotmeshAPI) RequestTransfer(
 
 }
 
+func (dm *DotmeshAPI) parseVolumeDetails(remote Remote, peer, direction, localFilesystemName, localBranchName, remoteFilesystemName, remoteBranchName string) (string, string, string, string, string, string, error) {
+	var err error
+	if direction == "push" {
+		// We are pushing, so if no local filesystem/branch is
+		// specified, take the current one.
+		if localFilesystemName == "" {
+			localFilesystemName, err = dm.Configuration.CurrentVolume()
+			if err != nil {
+				return "", "", "", "", "", "", err
+			}
+		}
+
+		if localBranchName == "" {
+			localBranchName, err = dm.Configuration.CurrentBranch()
+			if err != nil {
+				return "", "", "", "", "", "", err
+			}
+		}
+	} else if direction == "pull" {
+		// We are pulling, so if no local filesystem/branch is
+		// specified, we take the remote name but strip it of its
+		// namespace. So if we pull "bob/apples", we pull into "apples",
+		// which is really "admin/apples".
+		if localFilesystemName == "" && remoteFilesystemName != "" {
+			_, localFilesystemName, err = ParseNamespacedVolume(remoteFilesystemName)
+			if err != nil {
+				return "", "", "", "", "", "", err
+			}
+		}
+	}
+
+	// Split the local volume name's namespace out
+	localNamespace, localVolume, err := ParseNamespacedVolume(localFilesystemName)
+	if err != nil {
+		return "", "", "", "", "", "", err
+	}
+
+	// Guess defaults for the remote filesystem
+	var remoteNamespace, remoteVolume string
+	if remoteFilesystemName == "" {
+		// No remote specified. Do we already have a default configured?
+		defaultRemoteNamespace, defaultRemoteVolume, ok := dm.Configuration.DefaultRemoteVolumeFor(peer, localNamespace, localVolume)
+		if ok {
+			// If so, use it
+			remoteNamespace = defaultRemoteNamespace
+			remoteVolume = defaultRemoteVolume
+		} else {
+			// If not, default to the un-namespaced local filesystem name.
+			// This causes it to default into the user's own namespace
+			// when we parse the name, too.
+			remoteNamespace = remote.DefaultNamespace()
+			remoteVolume = localVolume
+		}
+	} else {
+		// Default namespace for remote volume is the username on this remote
+		remoteNamespace, remoteVolume, err = ParseNamespacedVolumeWithDefault(remoteFilesystemName, remote.DefaultNamespace())
+		if err != nil {
+			return "", "", "", "", "", "", err
+		}
+	}
+
+	// Remember default remote if there isn't already one
+	_, _, ok := dm.Configuration.DefaultRemoteVolumeFor(peer, localNamespace, localVolume)
+	if !ok {
+		dm.Configuration.SetDefaultRemoteVolumeFor(peer, localNamespace, localVolume, remoteNamespace, remoteVolume)
+	}
+
+	if remoteBranchName == "" {
+		remoteBranchName = localBranchName
+	}
+
+	if remoteBranchName != "" && remoteVolume == "" {
+		return "", "", "", "", "", "", fmt.Errorf(
+			"It's dubious to specify a remote branch name " +
+				"without specifying a remote filesystem name.",
+		)
+	}
+	return localNamespace, localVolume, localBranchName, remoteNamespace, remoteVolume, remoteBranchName, nil
+}
+
 func (dm *DotmeshAPI) RequestS3SubsetTransfer(
-	peer,
-	direction,
-	localFilesystemName,
-	remoteVolume string,
+	direction, peer,
+	localFilesystemName, localBranchName,
+	remoteFilesystemName, remoteBranchName string,
 	prefixes []string,
 ) (string, error) {
 	var transferId string
-	localNamespace, localVolume, err := ParseNamespacedVolume(localFilesystemName)
-	if err != nil {
-		return "", err
-	}
+
 	connectionInitiator := dm.Configuration.CurrentRemote
 	client, err := dm.Configuration.ClusterFromRemote(connectionInitiator, dm.verbose)
 	if err != nil {
@@ -1109,6 +1112,10 @@ func (dm *DotmeshAPI) RequestS3SubsetTransfer(
 	if err != nil {
 		return "", err
 	}
+	localNamespace, localVolume, localBranchName, _, remoteVolume, _, err := dm.parseVolumeDetails(remote, peer, direction, localFilesystemName, localBranchName, remoteFilesystemName, remoteBranchName)
+	if err != nil {
+		return "", err
+	}
 	s3Remote, ok := remote.(*S3Remote)
 	if ok {
 
@@ -1120,7 +1127,7 @@ func (dm *DotmeshAPI) RequestS3SubsetTransfer(
 			Direction:       direction,
 			LocalNamespace:  localNamespace,
 			LocalName:       localVolume,
-			LocalBranchName: "",
+			LocalBranchName: localBranchName,
 			RemoteName:      remoteVolume,
 			// TODO add TargetSnapshot here, to support specifying "push to a given
 			// snapshot" rather than just "push all snapshots up to the latest"