From 8868d3938a460f193e2b851e48da9d72b881486a Mon Sep 17 00:00:00 2001 From: Tom Huang Date: Tue, 11 Jan 2022 13:23:17 -0500 Subject: [PATCH 1/2] Update file close operation to not use defer and add test case for CopyFromPath Signed-off-by: Tom Huang --- controllers/storage.go | 6 ++- controllers/storage_test.go | 105 ++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/controllers/storage.go b/controllers/storage.go index d765e4303..860448405 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -355,8 +355,10 @@ func (s *Storage) CopyFromPath(artifact *sourcev1.Artifact, path string) (err er if err != nil { return err } - defer f.Close() - return s.Copy(artifact, f) + if err = s.Copy(artifact, f); err != nil { + return err + } + return f.Close() } // CopyToPath copies the contents in the (sub)path of the given artifact to the given path. diff --git a/controllers/storage_test.go b/controllers/storage_test.go index 8da8d49df..26a735856 100644 --- a/controllers/storage_test.go +++ b/controllers/storage_test.go @@ -259,3 +259,108 @@ func TestStorageRemoveAllButCurrent(t *testing.T) { } }) } + +func TestStorageCopyFromPath(t *testing.T) { + type File struct { + Name string + Content []byte + } + + dir, err := createStoragePath() + if err != nil { + t.Fatal(err) + } + t.Cleanup(cleanupStoragePath(dir)) + + storage, err := NewStorage(dir, "hostname", time.Minute) + if err != nil { + t.Fatalf("error while bootstrapping storage: %v", err) + } + + createFile := func(file *File) (absPath string, err error) { + defer func() { + if err != nil && dir != "" { + os.RemoveAll(dir) + } + }() + dir, err = os.MkdirTemp("", "test-files-") + if err != nil { + return + } + absPath = filepath.Join(dir, file.Name) + if err = os.MkdirAll(filepath.Dir(absPath), 0755); err != nil { + return + } + f, err := os.Create(absPath) + if err != nil { + return "", fmt.Errorf("could not create file %q: %w", absPath, err) + } + if n, err := f.Write(file.Content); err != nil { + f.Close() + return "", fmt.Errorf("could not write %d bytes to file %q: %w", n, f.Name(), err) + } + f.Close() + return + } + + matchFile := func(t *testing.T, storage *Storage, artifact sourcev1.Artifact, file *File, wantErr bool) { + c, err := os.ReadFile(storage.LocalPath(artifact)) + if err != nil { + t.Fatalf("failed reading file: %v", err) + } + if (string(c) != string(file.Content)) != wantErr { + t.Errorf("artifact content does not match, got: %q, want: %q", string(c), string(file.Content)) + } + } + + tests := []struct { + name string + file *File + want *File + wantErr bool + }{ + { + name: "content match", + file: &File{ + Name: "manifest.yaml", + Content: []byte(`contents`), + }, + want: &File{ + Name: "manifest.yaml", + Content: []byte(`contents`), + }, + }, + { + name: "content not match", + file: &File{ + Name: "manifest.yaml", + Content: []byte(`contents`), + }, + want: &File{ + Name: "manifest.yaml", + Content: []byte(`bad contents`), + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + absPath, err := createFile(tt.file) + if err != nil { + t.Error(err) + return + } + defer os.RemoveAll(absPath) + artifact := sourcev1.Artifact{ + Path: filepath.Join(randStringRunes(10), randStringRunes(10), randStringRunes(10)), + } + if err := storage.MkdirAll(artifact); err != nil { + t.Fatalf("artifact directory creation failed: %v", err) + } + if err := storage.CopyFromPath(&artifact, absPath); err != nil { + t.Errorf("CopyFromPath() error = %v", err) + } + matchFile(t, storage, artifact, tt.want, tt.wantErr) + }) + } +} From 5bb428349ea957169c7073905bee51cb0b932033 Mon Sep 17 00:00:00 2001 From: Tom Huang Date: Tue, 11 Jan 2022 15:50:25 -0500 Subject: [PATCH 2/2] proper file close operation based on feedback Signed-off-by: Tom Huang --- controllers/storage.go | 11 +++++++---- controllers/storage_test.go | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/controllers/storage.go b/controllers/storage.go index 860448405..5c1f7be02 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -355,10 +355,13 @@ func (s *Storage) CopyFromPath(artifact *sourcev1.Artifact, path string) (err er if err != nil { return err } - if err = s.Copy(artifact, f); err != nil { - return err - } - return f.Close() + defer func() { + if cerr := f.Close(); cerr != nil && err == nil { + err = cerr + } + }() + err = s.Copy(artifact, f) + return err } // CopyToPath copies the contents in the (sub)path of the given artifact to the given path. diff --git a/controllers/storage_test.go b/controllers/storage_test.go index 26a735856..57dae538a 100644 --- a/controllers/storage_test.go +++ b/controllers/storage_test.go @@ -303,21 +303,21 @@ func TestStorageCopyFromPath(t *testing.T) { return } - matchFile := func(t *testing.T, storage *Storage, artifact sourcev1.Artifact, file *File, wantErr bool) { + matchFile := func(t *testing.T, storage *Storage, artifact sourcev1.Artifact, file *File, expectMismatch bool) { c, err := os.ReadFile(storage.LocalPath(artifact)) if err != nil { t.Fatalf("failed reading file: %v", err) } - if (string(c) != string(file.Content)) != wantErr { - t.Errorf("artifact content does not match, got: %q, want: %q", string(c), string(file.Content)) + if (string(c) != string(file.Content)) != expectMismatch { + t.Errorf("artifact content does not match and not expecting mismatch, got: %q, want: %q", string(c), string(file.Content)) } } tests := []struct { - name string - file *File - want *File - wantErr bool + name string + file *File + want *File + expectMismatch bool }{ { name: "content match", @@ -338,9 +338,9 @@ func TestStorageCopyFromPath(t *testing.T) { }, want: &File{ Name: "manifest.yaml", - Content: []byte(`bad contents`), + Content: []byte(`mismatch contents`), }, - wantErr: true, + expectMismatch: true, }, } for _, tt := range tests { @@ -360,7 +360,7 @@ func TestStorageCopyFromPath(t *testing.T) { if err := storage.CopyFromPath(&artifact, absPath); err != nil { t.Errorf("CopyFromPath() error = %v", err) } - matchFile(t, storage, artifact, tt.want, tt.wantErr) + matchFile(t, storage, artifact, tt.want, tt.expectMismatch) }) } }