Address PR comments

Remove ability to override subdir from run command given subdirs now on a per input basis

cli/data/get.go

@@ -36,7 +36,7 @@ var (
 	getCommitPath string
 	getBucket     string
 	getFiles      []string
-	getSubdir     bool
+	getSubdir     string
 )
 
 const (
@@ -53,7 +53,7 @@ var getCmd = &cobra.Command{
 
 Example:
 
-$ paddle data get -b experimental --bucket roo-pipeline --subdir true trained-model/version1 dest/path
+$ paddle data get -b experimental --bucket roo-pipeline --subdir version1 trained-model/version1 dest/path
 $ paddle data get -b experimental --bucket roo-pipeline --files file1.csv,file2.csv trained-model/version1 dest/path
 `,
 	Run: func(cmd *cobra.Command, args []string) {
@@ -78,10 +78,10 @@ func init() {
 	getCmd.Flags().StringVar(&getBucket, "bucket", "", "Bucket to use")
 	getCmd.Flags().StringVarP(&getCommitPath, "path", "p", "HEAD", "Path to fetch (instead of HEAD)")
 	getCmd.Flags().StringSliceVarP(&getFiles, "files", "f", []string{}, "A list of files to download separated by comma")
-	getCmd.Flags().BoolVarP(&getSubdir, "subdir", "d", false, "Add step name as export path subdirectory")
+	getCmd.Flags().StringVarP(&getSubdir, "subdir", "d", "", "Custom subfolder name for export path")
 }
 
-func copyPathToDestination(source S3Path, destination string, files []string, subdir bool) {
+func copyPathToDestination(source S3Path, destination string, files []string, subdir string) {
 	session := session.Must(session.NewSessionWithOptions(session.Options{
 		SharedConfigState: session.SharedConfigEnable,
 	}))
@@ -96,9 +96,8 @@ func copyPathToDestination(source S3Path, destination string, files []string, su
 	if !strings.HasSuffix(source.path, "/") {
 		source.path += "/"
 	}
-	if subdir {
-		subdirName := strings.Split(source.path, "/")[0]
-		destination = parseDestination(destination, subdirName)
+	if subdir != "" {
+		destination = parseDestination(destination, subdir)
 	}
 
 	fmt.Println("Copying " + source.path + " to " + destination)
@@ -119,11 +118,11 @@ func readHEAD(session *session.Session, source S3Path) string {
 	return buf.String()
 }
 
-func parseDestination(destination string, subdirName string) string {
+func parseDestination(destination string, subdir string) string {
 	if !strings.HasPrefix(destination, "/") {
-		destination += "/" + subdirName
+		destination += "/" + subdir
 	} else {
-		destination += subdirName
+		destination += subdir
 	}
 	return destination
 }

cli/pipeline/pipeline_definition.go

@@ -13,13 +13,13 @@ type PipelineDefinitionStep struct {
 	Version string `yaml:"version" json:"version"`
 	Branch  string `yaml:"branch" json:"branch"`
 	Image   string `yaml:"image" json:"image"`
-	Subdir  bool   `yaml:"subdir" json:"subdir"`
 	Inputs  []struct {
 		Step    string `yaml:"step" json:"step"`
 		Version string `yaml:"version" json:"version"`
 		Branch  string `yaml:"branch" json:"branch"`
 		Path    string `yaml:"path" json:"path"`
 		Bucket  string `yaml:"bucket" json:"bucket"`
+		Subdir  string `yaml:"subdir" json:"subdir"`
 	} `yaml:"inputs" json:"inputs"`
 	Commands  []string `yaml:"commands" json:"commands"`
 	Resources struct {
@@ -66,12 +66,6 @@ func (p *PipelineDefinitionStep) OverrideTag(tag string) {
 	}
 }
 
-func (p *PipelineDefinitionStep) OverrideSubdir(subdir bool) {
-	if subdir {
-		p.Subdir = subdir
-	}
-}
-
 func (p *PipelineDefinitionStep) OverrideVersion(version string, overrideInputs bool) {
 	if version != "" {
 		p.Version = version

cli/pipeline/pipeline_definition_test.go

@@ -20,12 +20,9 @@ func TestParsePipeline(t *testing.T) {
 		t.Errorf("Expected bucket to be canoe-sample-pipeline, got %s", pipeline.Bucket)
 	}
 
-	if pipeline.Steps[0].Subdir != false {
-		t.Errorf("expected first step.subdir to be false but got %t", pipeline.Steps[0].Subdir)
-	}
-
-	if pipeline.Steps[1].Subdir != true {
-		t.Errorf("expected second step.subdir to be true but got %t", pipeline.Steps[1].Subdir)
+	subdir := pipeline.Steps[1].Inputs[0].Subdir
+	if subdir != "step1-version1" {
+		t.Errorf("expected second step input subdir to be 'step1-version1' but got %s", subdir)
 	}
 }
 
@@ -49,26 +46,6 @@ func TestOverrideTag(t *testing.T) {
 	}
 }
 
-func TestOverrideSubdir(t *testing.T) {
-	data, err := ioutil.ReadFile("test/sample_steps_passing.yml")
-	if err != nil {
-		panic(err.Error())
-	}
-	pipeline := ParsePipeline(data)
-
-	pipeline.Steps[0].OverrideSubdir(true)
-
-	if pipeline.Steps[0].Subdir != true {
-		t.Errorf("expected subdir to be overriden to true but it is still %t", pipeline.Steps[0].Subdir)
-	}
-
-	pipeline.Steps[1].OverrideSubdir(false)
-
-	if pipeline.Steps[1].Subdir != true {
-		t.Errorf("expected subdir not to be overriden from true but it is now %t", pipeline.Steps[1].Subdir)
-	}
-}
-
 func TestOverrideVersion(t *testing.T) {
 	data, err := ioutil.ReadFile("test/sample_steps_passing.yml")
 	if err != nil {

cli/pipeline/run.go

@@ -34,7 +34,6 @@ type runCmdFlagsStruct struct {
 	StepName           string
 	BucketName         string
 	ImageTag           string
-	Subdir             bool
 	StepBranch         string
 	StepVersion        string
 	OverrideInputs     bool
@@ -75,7 +74,6 @@ func init() {
 	runCmd.Flags().StringVarP(&runCmdFlags.StepName, "step", "s", "", "Single step to execute")
 	runCmd.Flags().StringVarP(&runCmdFlags.BucketName, "bucket", "b", "", "Bucket name")
 	runCmd.Flags().StringVarP(&runCmdFlags.ImageTag, "tag", "t", "", "Image tag (overrides the one defined in the pipeline)")
-	runCmd.Flags().BoolVarP(&runCmdFlags.Subdir, "subdir", "d", false, "Add step name as export path subdirectory")
 	runCmd.Flags().StringVarP(&runCmdFlags.StepBranch, "step-branch", "B", "", "Step branch (overrides the one defined in the pipeline)")
 	runCmd.Flags().StringVarP(&runCmdFlags.StepVersion, "step-version", "V", "", "Step version (overrides the one defined in the pipeline)")
 	runCmd.Flags().BoolVarP(&runCmdFlags.TailLogs, "logs", "l", true, "Tail logs")
@@ -113,9 +111,6 @@ func runPipeline(path string, flags *runCmdFlagsStruct) {
 		if flags.ImageTag != "" {
 			step.OverrideTag(flags.ImageTag)
 		}
-		if flags.Subdir {
-			step.OverrideSubdir(flags.Subdir)
-		}
 		if flags.StepBranch != "" {
 			step.OverrideBranch(flags.StepBranch, flags.OverrideInputs)
 		}

cli/pipeline/template.go

@@ -132,7 +132,7 @@ spec:
         - "-c"
         - "mkdir -p $INPUT_PATH $OUTPUT_PATH &&
           {{ range $index, $input := .Step.Inputs }}
-          paddle data get {{ $input.Step }}/{{ $input.Version }} $INPUT_PATH -b {{ $input.Branch | sanitizeName }} -p {{ $input.Path }} -d {{ .Step.Subdir }} {{ $input.Bucket | bucketParam }} &&
+          paddle data get {{ $input.Step }}/{{ $input.Version }} $INPUT_PATH -b {{ $input.Branch | sanitizeName }} -p {{ $input.Path }} -d {{ $input.Subdir }} {{ $input.Bucket | bucketParam }} &&
           {{ end }}
           touch /data/first-step.txt &&
           echo first step finished &&

cli/pipeline/template_test.go

@@ -2,6 +2,7 @@ package pipeline
 
 import (
 	"io/ioutil"
+	"strings"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
@@ -15,20 +16,24 @@ func TestCompileTemplate(t *testing.T) {
 	}
 	pipeline := ParsePipeline(data)
 
-	podDefinition := NewPodDefinition(pipeline, &pipeline.Steps[0])
+	podDefinition := NewPodDefinition(pipeline, &pipeline.Steps[1])
 
 	stepPodBuffer := podDefinition.compile()
 
 	pod := &v1.Pod{}
 	yaml.NewYAMLOrJSONDecoder(stepPodBuffer, 4096).Decode(pod)
 
-	if pod.Name != "sample-steps-passing-version1-step1-master" {
+	if pod.Name != "sample-steps-passing-version1a-step2-master" {
 		t.Errorf("Pod name is %s", pod.Name)
 	}
 
 	if pod.Spec.Containers[0].Image != pipeline.Steps[0].Image {
 		t.Errorf("First image is %s", pod.Spec.Containers[0].Image)
 	}
+
+	if !strings.Contains(pod.Spec.Containers[1].Command[2], "-d step1-version1") {
+		t.Errorf("Expected paddle get command to contain -d step1-version1, but it did not")
+	}
 }
 
 func TestSecrets(t *testing.T) {

cli/pipeline/test/sample_steps_passing.yml

@@ -10,7 +10,6 @@ steps:
     inputs: []
     image: 219541440308.dkr.ecr.eu-west-1.amazonaws.com/paddlecontainer:latest
     branch: master
-    subdir: false
     commands:
       - echo executing sample-pipeline-data > ${OUTPUT_PATH}/sample-pipeline-data.txt
     resources:
@@ -26,9 +25,9 @@ steps:
         version: version1
         branch: master
         path: HEAD
+        subdir: 'step1-version1'
     image: 219541440308.dkr.ecr.eu-west-1.amazonaws.com/paddlecontainer:latest
     branch: master
-    subdir: true
     commands:
       - echo executing sample-pipeline-data > ${OUTPUT_PATH}/sample-pipeline-data-model.txt
     resources: