From ffb281a5567666db68a5acab03ba7a0188954bf8 Mon Sep 17 00:00:00 2001 From: Anes Benmerzoug Date: Sat, 21 Sep 2019 02:08:02 +0200 Subject: [PATCH] Small code cleanup and add tests (#1562) --- cmd/argo/commands/submit.go | 43 ++------- cmd/argo/commands/template/create.go | 44 ++------- util/file/fileutil_test.go | 2 +- workflow/util/util.go | 81 ++++++++++++++-- workflow/util/util_test.go | 137 ++++++++++++++++++++++++++- 5 files changed, 228 insertions(+), 79 deletions(-) diff --git a/cmd/argo/commands/submit.go b/cmd/argo/commands/submit.go index da0a5d4d1fde..acb8782dfdff 100644 --- a/cmd/argo/commands/submit.go +++ b/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 { diff --git a/cmd/argo/commands/template/create.go b/cmd/argo/commands/template/create.go index c9371bdfd63a..d28befd999f5 100644 --- a/cmd/argo/commands/template/create.go +++ b/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 { diff --git a/util/file/fileutil_test.go b/util/file/fileutil_test.go index 5ec5dce60233..d1a3bfe80047 100644 --- a/util/file/fileutil_test.go +++ b/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\":" + diff --git a/workflow/util/util.go b/workflow/util/util.go index 169ca097e5fb..841e5b55df9f 100644 --- a/workflow/util/util.go +++ b/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 +} diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go index 550affc03af2..e76f76a7a426 100644 --- a/workflow/util/util_test.go +++ b/workflow/util/util_test.go @@ -1,20 +1,53 @@ package util import ( + "io/ioutil" + "os" + "path/filepath" "testing" + "github.com/ghodss/yaml" + wfv1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1" + fakeClientset "github.com/argoproj/argo/pkg/client/clientset/versioned/fake" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// TestSubmitDryRun +func TestSubmitDryRun(t *testing.T) { + + workflowName := "test-dry-run" + workflowYaml := ` +apiVersion: argoproj.io/v1alpha1 +kind: Workflow +metadata: + name: ` + workflowName + ` +spec: + entrypoint: whalesay + templates: + - name: whalesay + container: + image: docker/whalesay:latest + command: [cowsay] + args: ["hello world"] +` + wf := unmarshalWF(workflowYaml) + newWf := wf.DeepCopy() + wfClientSet := fakeClientset.NewSimpleClientset() + newWf, err := SubmitWorkflow(nil, wfClientSet, "test-namespace", newWf, &SubmitOpts{DryRun: true}) + assert.Nil(t, err) + assert.Equal(t, wf.Spec, newWf.Spec) + assert.Equal(t, wf.Status, newWf.Status) +} + // TestResubmitWorkflowWithOnExit ensures we do not carry over the onExit node even if successful func TestResubmitWorkflowWithOnExit(t *testing.T) { wfName := "test-wf" onExitName := wfName + ".onExit" wf := wfv1.Workflow{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-wf", + Name: wfName, }, Status: wfv1.WorkflowStatus{ Phase: wfv1.NodeFailed, @@ -33,3 +66,105 @@ func TestResubmitWorkflowWithOnExit(t *testing.T) { _, ok := newWF.Status.Nodes[newWFOneExitID] assert.False(t, ok) } + +// TestReadFromSingleorMultiplePath ensures we can read the content of a single file or multiple files correctly using the ReadFromFilePathsOrUrls function +func TestReadFromSingleorMultiplePath(t *testing.T) { + tests := map[string]struct { + fileNames []string + contents []string + }{ + "singleFile": { + fileNames: []string{"singleFile"}, + contents: []string{"test file's content"}, + }, + "multipleFiles": { + fileNames: []string{"file1", "file2"}, + contents: []string{"file1 content", "file2 content"}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + dir, err := ioutil.TempDir("", name) + if err != nil { + t.Error("Could not create temporary directory") + } + defer os.RemoveAll(dir) + var filePaths []string + for i := range tc.fileNames { + content := []byte(tc.contents[i]) + tmpfn := filepath.Join(dir, tc.fileNames[i]) + filePaths = append(filePaths, tmpfn) + err := ioutil.WriteFile(tmpfn, content, 0666) + if err != nil { + t.Error("Could not write to temporary file") + } + } + body, err := ReadFromFilePathsOrUrls(filePaths...) + assert.Equal(t, len(body), len(filePaths)) + assert.Nil(t, err) + for i := range body { + assert.Equal(t, body[i], []byte(tc.contents[i])) + } + }) + } +} + +// TestReadFromSingleorMultiplePathErrorHandling ensures that an error is returned if there is any error while reading files or urls +func TestReadFromSingleorMultiplePathErrorHandling(t *testing.T) { + tests := map[string]struct { + fileNames []string + contents []string + exists []bool + }{ + "nonExistingFile": { + fileNames: []string{"nonExistingFile"}, + contents: []string{"this content should not exist"}, + exists: []bool{false}, + }, + "multipleNonExistingFiles": { + fileNames: []string{"file1", "file2"}, + contents: []string{"this content should not exist", "this content should not exist"}, + exists: []bool{false, false}, + }, + "mixedExistingAndNonExistingFiles": { + fileNames: []string{"file1", "file2", "file3", "file4"}, + contents: []string{"actual file content", "", "", "actual file content 2"}, + exists: []bool{true, false, false, true}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + dir, err := ioutil.TempDir("", name) + if err != nil { + t.Error("Could not create temporary directory") + } + defer os.RemoveAll(dir) + var filePaths []string + for i := range tc.fileNames { + content := []byte(tc.contents[i]) + tmpfn := filepath.Join(dir, tc.fileNames[i]) + filePaths = append(filePaths, tmpfn) + if tc.exists[i] { + err := ioutil.WriteFile(tmpfn, content, 0666) + if err != nil { + t.Error("Could not write to temporary file") + } + } + } + body, err := ReadFromFilePathsOrUrls(filePaths...) + assert.NotNil(t, err) + assert.Equal(t, len(body), 0) + }) + } +} + +func unmarshalWF(yamlStr string) *wfv1.Workflow { + var wf wfv1.Workflow + err := yaml.Unmarshal([]byte(yamlStr), &wf) + if err != nil { + panic(err) + } + return &wf +}