Small code cleanup and add tests (#1562)

cmd/argo/commands/submit.go

@@ -1,10 +1,7 @@
 package commands
 
 import (
-	"bufio"
-	"io/ioutil"
 	"log"
-	"net/http"
 	"os"
 	"strconv"
 
@@ -14,7 +11,6 @@ import (
 	apimachineryversion "k8s.io/apimachinery/pkg/version"
 
 	wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
-	cmdutil "github.com/argoproj/argo/util/cmd"
 	"github.com/argoproj/argo/workflow/common"
 	"github.com/argoproj/argo/workflow/util"
 )
@@ -81,37 +77,16 @@ func SubmitWorkflows(filePaths []string, submitOpts *util.SubmitOpts, cliOpts *c
 		cliOpts = &cliSubmitOpts{}
 	}
 	defaultWFClient := InitWorkflowClient()
+
+	fileContents, err := util.ReadManifest(filePaths...)
+	if err != nil {
+		log.Fatal(err)
+	}
+
 	var workflows []wfv1.Workflow
-	if len(filePaths) == 1 && filePaths[0] == "-" {
-		reader := bufio.NewReader(os.Stdin)
-		body, err := ioutil.ReadAll(reader)
-		if err != nil {
-			log.Fatal(err)
-		}
-		workflows = unmarshalWorkflows(body, cliOpts.strict)
-	} else {
-		for _, filePath := range filePaths {
-			var body []byte
-			var err error
-			if cmdutil.IsURL(filePath) {
-				response, err := http.Get(filePath)
-				if err != nil {
-					log.Fatal(err)
-				}
-				body, err = ioutil.ReadAll(response.Body)
-				_ = response.Body.Close()
-				if err != nil {
-					log.Fatal(err)
-				}
-			} else {
-				body, err = ioutil.ReadFile(filePath)
-				if err != nil {
-					log.Fatal(err)
-				}
-			}
-			wfs := unmarshalWorkflows(body, cliOpts.strict)
-			workflows = append(workflows, wfs...)
-		}
+	for _, body := range fileContents {
+		wfs := unmarshalWorkflows(body, cliOpts.strict)
+		workflows = append(workflows, wfs...)
 	}
 
 	if cliOpts.watch {

cmd/argo/commands/template/create.go

@@ -1,18 +1,15 @@
 package template
 
 import (
-	"bufio"
-	"io/ioutil"
 	"log"
-	"net/http"
 	"os"
 
 	"github.com/argoproj/pkg/json"
 	"github.com/spf13/cobra"
 
 	wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
-	cmdutil "github.com/argoproj/argo/util/cmd"
 	"github.com/argoproj/argo/workflow/common"
+	"github.com/argoproj/argo/workflow/util"
 	"github.com/argoproj/argo/workflow/validate"
 )
 
@@ -47,37 +44,16 @@ func CreateWorkflowTemplates(filePaths []string, cliOpts *cliCreateOpts) {
 		cliOpts = &cliCreateOpts{}
 	}
 	defaultWFTmplClient := InitWorkflowTemplateClient()
+
+	fileContents, err := util.ReadManifest(filePaths...)
+	if err != nil {
+		log.Fatal(err)
+	}
+
 	var workflowTemplates []wfv1.WorkflowTemplate
-	if len(filePaths) == 1 && filePaths[0] == "-" {
-		reader := bufio.NewReader(os.Stdin)
-		body, err := ioutil.ReadAll(reader)
-		if err != nil {
-			log.Fatal(err)
-		}
-		workflowTemplates = unmarshalWorkflowTemplates(body, cliOpts.strict)
-	} else {
-		for _, filePath := range filePaths {
-			var body []byte
-			var err error
-			if cmdutil.IsURL(filePath) {
-				response, err := http.Get(filePath)
-				if err != nil {
-					log.Fatal(err)
-				}
-				body, err = ioutil.ReadAll(response.Body)
-				_ = response.Body.Close()
-				if err != nil {
-					log.Fatal(err)
-				}
-			} else {
-				body, err = ioutil.ReadFile(filePath)
-				if err != nil {
-					log.Fatal(err)
-				}
-			}
-			wftmpls := unmarshalWorkflowTemplates(body, cliOpts.strict)
-			workflowTemplates = append(workflowTemplates, wftmpls...)
-		}
+	for _, body := range fileContents {
+		wftmpls := unmarshalWorkflowTemplates(body, cliOpts.strict)
+		workflowTemplates = append(workflowTemplates, wftmpls...)
 	}
 
 	if len(workflowTemplates) == 0 {

util/file/fileutil_test.go

@@ -11,7 +11,7 @@ import (
 	"github.com/argoproj/argo/util/file"
 )
 
-// TestResubmitWorkflowWithOnExit ensures we do not carry over the onExit node even if successful
+// TestCompressContentString ensures compressing then decompressing a content string works as expected
 func TestCompressContentString(t *testing.T) {
 	content := "{\"pod-limits-rrdm8-591645159\":{\"id\":\"pod-limits-rrdm8-591645159\",\"name\":\"pod-limits-rrdm8[0]." +
 		"run-pod(0:0)\",\"displayName\":\"run-pod(0:0)\",\"type\":\"Pod\",\"templateName\":\"run-pod\",\"phase\":" +

workflow/util/util.go

@@ -1,11 +1,13 @@
 package util
 
 import (
+	"bufio"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"math/rand"
 	"net/http"
+	"os"
 	"regexp"
 	"strings"
 	"time"
@@ -197,12 +199,7 @@ func SubmitWorkflow(wfIf v1alpha1.WorkflowInterface, wfClientset wfclientset.Int
 			var body []byte
 			var err error
 			if cmdutil.IsURL(opts.ParameterFile) {
-				response, err := http.Get(opts.ParameterFile)
-				if err != nil {
-					return nil, errors.InternalWrapError(err)
-				}
-				body, err = ioutil.ReadAll(response.Body)
-				_ = response.Body.Close()
+				body, err = ReadFromUrl(opts.ParameterFile)
 				if err != nil {
 					return nil, errors.InternalWrapError(err)
 				}
@@ -258,14 +255,14 @@ func SubmitWorkflow(wfIf v1alpha1.WorkflowInterface, wfClientset wfclientset.Int
 		return nil, err
 	}
 
-	if opts.ServerDryRun {
+	if opts.DryRun {
+		return wf, nil
+	} else if opts.ServerDryRun {
 		wf, err := CreateServerDryRun(wf, wfClientset)
 		if err != nil {
 			return nil, err
 		}
 		return wf, err
-	} else if opts.DryRun {
-		return wf, nil
 	} else {
 		return wfIf.Create(wf)
 	}
@@ -535,6 +532,7 @@ func IsWorkflowSuspended(wf *wfv1.Workflow) bool {
 	return false
 }
 
+// IsWorkflowTerminated returns whether or not a workflow is considered terminated
 func IsWorkflowTerminated(wf *wfv1.Workflow) bool {
 	if wf.Spec.ActiveDeadlineSeconds != nil && *wf.Spec.ActiveDeadlineSeconds == 0 {
 		return true
@@ -583,3 +581,68 @@ func DecompressWorkflow(wf *wfv1.Workflow) error {
 	}
 	return nil
 }
+
+// Reads from stdin
+func ReadFromStdin() ([]byte, error) {
+	reader := bufio.NewReader(os.Stdin)
+	body, err := ioutil.ReadAll(reader)
+	if err != nil {
+		return []byte{}, err
+	}
+	return body, err
+}
+
+// Reads the content of a url
+func ReadFromUrl(url string) ([]byte, error) {
+	response, err := http.Get(url)
+	if err != nil {
+		return nil, err
+	}
+	body, err := ioutil.ReadAll(response.Body)
+	_ = response.Body.Close()
+	if err != nil {
+		return nil, err
+	}
+	return body, err
+}
+
+// ReadFromFilePathsOrUrls reads the content of a single or a list of file paths and/or urls
+func ReadFromFilePathsOrUrls(filePathsOrUrls ...string) ([][]byte, error) {
+	var fileContents [][]byte
+	var body []byte
+	var err error
+	for _, filePathOrUrl := range filePathsOrUrls {
+		if cmdutil.IsURL(filePathOrUrl) {
+			body, err = ReadFromUrl(filePathOrUrl)
+			if err != nil {
+				return [][]byte{}, err
+			}
+		} else {
+			body, err = ioutil.ReadFile(filePathOrUrl)
+			if err != nil {
+				return [][]byte{}, err
+			}
+		}
+		fileContents = append(fileContents, body)
+	}
+	return fileContents, err
+}
+
+// ReadManifest reads from stdin, a single file/url, or a list of files and/or urls
+func ReadManifest(manifestPaths ...string) ([][]byte, error) {
+	var manifestContents [][]byte
+	var err error
+	if len(manifestPaths) == 1 && manifestPaths[0] == "-" {
+		body, err := ReadFromStdin()
+		if err != nil {
+			return [][]byte{}, err
+		}
+		manifestContents = append(manifestContents, body)
+	} else {
+		manifestContents, err = ReadFromFilePathsOrUrls(manifestPaths...)
+		if err != nil {
+			return [][]byte{}, err
+		}
+	}
+	return manifestContents, err
+}