From 70af28be47256c51beffcc3e37ee0a569b8b4b14 Mon Sep 17 00:00:00 2001 From: Chris Bednarski Date: Fri, 14 Aug 2015 17:34:04 -0700 Subject: [PATCH 1/5] Added cake fixture for testing file:/// downloads --- common/test-fixtures/fileurl/cake | 1 + 1 file changed, 1 insertion(+) create mode 100644 common/test-fixtures/fileurl/cake diff --git a/common/test-fixtures/fileurl/cake b/common/test-fixtures/fileurl/cake new file mode 100644 index 000000000..e800d1ffb --- /dev/null +++ b/common/test-fixtures/fileurl/cake @@ -0,0 +1 @@ +delicious chocolate cake From 424ee658669a465cd92b42ef030b245778facd93 Mon Sep 17 00:00:00 2001 From: Chris Bednarski Date: Fri, 14 Aug 2015 17:34:39 -0700 Subject: [PATCH 2/5] Added a log message when we use a local file instead of downloading one --- common/download.go | 1 + 1 file changed, 1 insertion(+) diff --git a/common/download.go b/common/download.go index 16c0724c3..184624ffe 100644 --- a/common/download.go +++ b/common/download.go @@ -117,6 +117,7 @@ func (d *DownloadClient) Get() (string, error) { var finalPath string if url.Scheme == "file" && !d.config.CopyFile { finalPath = url.Path + log.Printf("Using local file: %s", finalPath) // Remove forward slash on absolute Windows file URLs before processing if runtime.GOOS == "windows" && len(finalPath) > 0 && finalPath[0] == '/' { From 7ecfb057ff551ce2388bee36354a16f57b4ee4ef Mon Sep 17 00:00:00 2001 From: Chris Bednarski Date: Fri, 14 Aug 2015 17:37:57 -0700 Subject: [PATCH 3/5] Added test case to catch deleting local source file when checksum doesn't match --- common/download_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/common/download_test.go b/common/download_test.go index dc5bd29ed..f77956913 100644 --- a/common/download_test.go +++ b/common/download_test.go @@ -3,6 +3,7 @@ package common import ( "crypto/md5" "encoding/hex" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -338,3 +339,40 @@ func TestHashForType(t *testing.T) { t.Fatalf("fake hash is not nil") } } + +func TestDownloadFileUrl(t *testing.T) { + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("Unable to detect working directory: %s", err) + } + + // source_path is a file path and source is a network path + sourcePath := fmt.Sprintf("%s/test-fixtures/fileurl/%s", cwd, "cake") + source := fmt.Sprintf("file://" + sourcePath) + t.Logf("Trying to download %s", source) + + config := &DownloadConfig{ + Url: source, + // This should be wrong. We want to make sure we don't delete + Checksum: []byte("nope"), + Hash: HashForType("sha256"), + CopyFile: false, + } + + client := NewDownloadClient(config) + + filename, err := client.Get() + defer os.Remove(config.TargetPath) + if err != nil { + t.Fatalf("Failed to download test file") + } + + if sourcePath != filename { + t.Errorf("Filename doesn't match; expected %s got %s", sourcePath, filename) + } + + if _, err = os.Stat(sourcePath); err != nil { + t.Errorf("Could not stat source file: %s", sourcePath) + } + +} From 6e8c6a15ad01cd9dde6e0ea08f93132c7778658d Mon Sep 17 00:00:00 2001 From: Chris Bednarski Date: Fri, 14 Aug 2015 17:49:08 -0700 Subject: [PATCH 4/5] Implement fix, add comments so it's more apparent why we're doing special logic --- common/download.go | 11 +++++++++-- common/download_test.go | 16 ++++++++-------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/common/download.go b/common/download.go index 184624ffe..5f213968b 100644 --- a/common/download.go +++ b/common/download.go @@ -115,7 +115,10 @@ func (d *DownloadClient) Get() (string, error) { // Files when we don't copy the file are special cased. var f *os.File var finalPath string + sourcePath := "" if url.Scheme == "file" && !d.config.CopyFile { + // This is a special case where we use a source file that already exists + // locally and we don't make a copy. Normally we would copy or download. finalPath = url.Path log.Printf("Using local file: %s", finalPath) @@ -123,6 +126,8 @@ func (d *DownloadClient) Get() (string, error) { if runtime.GOOS == "windows" && len(finalPath) > 0 && finalPath[0] == '/' { finalPath = finalPath[1:len(finalPath)] } + // Keep track of the source so we can make sure not to delete this later + sourcePath = finalPath } else { finalPath = d.config.TargetPath @@ -150,8 +155,10 @@ func (d *DownloadClient) Get() (string, error) { var verify bool verify, err = d.VerifyChecksum(finalPath) if err == nil && !verify { - // Delete the file - os.Remove(finalPath) + // Only delete the file if we made a copy or downloaded it + if sourcePath != finalPath { + os.Remove(finalPath) + } err = fmt.Errorf( "checksums didn't match expected: %s", diff --git a/common/download_test.go b/common/download_test.go index f77956913..51f6f270c 100644 --- a/common/download_test.go +++ b/common/download_test.go @@ -340,6 +340,10 @@ func TestHashForType(t *testing.T) { } } +// TestDownloadFileUrl tests a special case where we use a local file for +// iso_url. In this case we can still verify the checksum but we should not +// delete the file if the checksum fails. Instead we'll just error and let the +// user fix the checksum. func TestDownloadFileUrl(t *testing.T) { cwd, err := os.Getwd() if err != nil { @@ -361,14 +365,10 @@ func TestDownloadFileUrl(t *testing.T) { client := NewDownloadClient(config) - filename, err := client.Get() - defer os.Remove(config.TargetPath) - if err != nil { - t.Fatalf("Failed to download test file") - } - - if sourcePath != filename { - t.Errorf("Filename doesn't match; expected %s got %s", sourcePath, filename) + // Verify that we fail to match the checksum + _, err = client.Get() + if err.Error() != "checksums didn't match expected: 6e6f7065" { + t.Fatalf("Unexpected failure; expected checksum not to match") } if _, err = os.Stat(sourcePath); err != nil { From 1764238c0b3c9aa8ad42268890e4acc3cc1670d0 Mon Sep 17 00:00:00 2001 From: Chris Bednarski Date: Wed, 19 Aug 2015 13:15:23 -0700 Subject: [PATCH 5/5] Added [DEBUG] prefix to log messages --- common/download.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/common/download.go b/common/download.go index 5f213968b..e4b4dc2e0 100644 --- a/common/download.go +++ b/common/download.go @@ -101,7 +101,7 @@ func (d *DownloadClient) Cancel() { func (d *DownloadClient) Get() (string, error) { // If we already have the file and it matches, then just return the target path. if verify, _ := d.VerifyChecksum(d.config.TargetPath); verify { - log.Println("Initial checksum matched, no download needed.") + log.Println("[DEBUG] Initial checksum matched, no download needed.") return d.config.TargetPath, nil } @@ -120,7 +120,7 @@ func (d *DownloadClient) Get() (string, error) { // This is a special case where we use a source file that already exists // locally and we don't make a copy. Normally we would copy or download. finalPath = url.Path - log.Printf("Using local file: %s", finalPath) + log.Printf("[DEBUG] Using local file: %s", finalPath) // Remove forward slash on absolute Windows file URLs before processing if runtime.GOOS == "windows" && len(finalPath) > 0 && finalPath[0] == '/' { @@ -143,7 +143,7 @@ func (d *DownloadClient) Get() (string, error) { return "", err } - log.Printf("Downloading: %s", url.String()) + log.Printf("[DEBUG] Downloading: %s", url.String()) err = d.downloader.Download(f, url) f.Close() if err != nil {