From 6b3844a64f5e5a198467ddae9081717a8e0a5927 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 20 Aug 2018 10:48:06 +0200 Subject: [PATCH 1/2] Revert "allow to use ISO images in-place v.s. copying them" --- builder/virtualbox/iso/builder.go | 1 - builder/vmware/iso/builder.go | 1 - common/download.go | 16 ++++----- common/download_test.go | 55 +++++-------------------------- common/iso_config.go | 1 - common/step_download.go | 16 ++------- 6 files changed, 20 insertions(+), 70 deletions(-) diff --git a/builder/virtualbox/iso/builder.go b/builder/virtualbox/iso/builder.go index 7cf3dd7e3..2ea19e70f 100644 --- a/builder/virtualbox/iso/builder.go +++ b/builder/virtualbox/iso/builder.go @@ -204,7 +204,6 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe ResultKey: "iso_path", TargetPath: b.config.TargetPath, Url: b.config.ISOUrls, - Inplace: b.config.Inplace, }, &vboxcommon.StepOutputDir{ Force: b.config.PackerForce, diff --git a/builder/vmware/iso/builder.go b/builder/vmware/iso/builder.go index d428d0184..0944db927 100644 --- a/builder/vmware/iso/builder.go +++ b/builder/vmware/iso/builder.go @@ -291,7 +291,6 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe ResultKey: "iso_path", TargetPath: b.config.TargetPath, Url: b.config.ISOUrls, - Inplace: b.config.Inplace, }, &vmwcommon.StepOutputDir{ Force: b.config.PackerForce, diff --git a/common/download.go b/common/download.go index 84776bee9..09457ef0b 100644 --- a/common/download.go +++ b/common/download.go @@ -38,10 +38,10 @@ type DownloadConfig struct { // DownloaderMap maps a schema to a Download. DownloaderMap map[string]Downloader - // Inplace indicates wether to copy file - // versus using it inplace. - // Inplace can only be true when referencing local files. - Inplace bool + // If true, this will copy even a local file to the target + // location. If false, then it will "download" the file by just + // returning the local path to the file. + CopyFile bool // The hashing implementation to use to checksum the downloaded file. Hash hash.Hash @@ -155,12 +155,12 @@ func (d *DownloadClient) Get() (string, error) { } local, ok := d.downloader.(LocalDownloader) - if !ok && d.config.Inplace { - d.config.Inplace = false + if !ok && !d.config.CopyFile { + d.config.CopyFile = true } // If we're copying the file, then just use the actual downloader - if d.config.Inplace == false { + if d.config.CopyFile { var f *os.File finalPath = d.config.TargetPath @@ -192,7 +192,7 @@ func (d *DownloadClient) Get() (string, error) { verify, err = d.VerifyChecksum(finalPath) if err == nil && !verify { // Only delete the file if we made a copy or downloaded it - if d.config.Inplace == false { + if d.config.CopyFile { os.Remove(finalPath) } diff --git a/common/download_test.go b/common/download_test.go index 9305e7db8..c9175b013 100644 --- a/common/download_test.go +++ b/common/download_test.go @@ -58,6 +58,7 @@ func TestDownloadClient_basic(t *testing.T) { client := NewDownloadClient(&DownloadConfig{ Url: ts.URL + "/basic.txt", TargetPath: tf.Name(), + CopyFile: true, }) path, err := client.Get() @@ -93,6 +94,7 @@ func TestDownloadClient_checksumBad(t *testing.T) { TargetPath: tf.Name(), Hash: HashForType("md5"), Checksum: checksum, + CopyFile: true, }) if _, err := client.Get(); err == nil { @@ -118,6 +120,7 @@ func TestDownloadClient_checksumGood(t *testing.T) { TargetPath: tf.Name(), Hash: HashForType("md5"), Checksum: checksum, + CopyFile: true, }) path, err := client.Get() @@ -149,6 +152,7 @@ func TestDownloadClient_checksumNoDownload(t *testing.T) { TargetPath: "./test-fixtures/root/another.txt", Hash: HashForType("md5"), Checksum: checksum, + CopyFile: true, }) path, err := client.Get() if err != nil { @@ -206,6 +210,7 @@ func TestDownloadClient_resume(t *testing.T) { client := NewDownloadClient(&DownloadConfig{ Url: ts.URL, TargetPath: tf.Name(), + CopyFile: true, }) path, err := client.Get() @@ -265,6 +270,7 @@ func TestDownloadClient_usesDefaultUserAgent(t *testing.T) { config := &DownloadConfig{ Url: server.URL, TargetPath: tf.Name(), + CopyFile: true, } client := NewDownloadClient(config) @@ -297,6 +303,7 @@ func TestDownloadClient_setsUserAgent(t *testing.T) { Url: server.URL, TargetPath: tf.Name(), UserAgent: "fancy user agent", + CopyFile: true, } client := NewDownloadClient(config) @@ -395,7 +402,7 @@ func TestDownloadFileUrl(t *testing.T) { // This should be wrong. We want to make sure we don't delete Checksum: []byte("nope"), Hash: HashForType("sha256"), - Inplace: true, + CopyFile: false, } client := NewDownloadClient(config) @@ -411,50 +418,6 @@ func TestDownloadFileUrl(t *testing.T) { } } -// TestDownloadFileUrl_inplace verifies that inplace setting is respected. -func TestDownloadFileUrl_inplace(t *testing.T) { - cwd, err := os.Getwd() - if err != nil { - t.Fatalf("Unable to detect working directory: %s", err) - } - cwd = filepath.ToSlash(cwd) - - // source_path is a file path and source is a network path - sourcePath := fmt.Sprintf("%s/test-fixtures/fileurl/%s", cwd, "cake") - - filePrefix := "file://" - if runtime.GOOS == "windows" { - filePrefix += "/" - } - - source := fmt.Sprintf(filePrefix + sourcePath) - t.Logf("Trying to download %s", source) - - config := &DownloadConfig{ - Url: source, - // This is correct. We want to make sure we don't delete - Checksum: []byte{96, 111, 25, 69, 248, 26, 2, 45, 14, 208, 189, 153, 237, 253, 79, 153, 8, 28, 28, 177, 249, 127, 174, 8, 114, 145, 238, 20, 233, 69, 230, 8}, - Hash: HashForType("sha256"), - Inplace: true, - } - - client := NewDownloadClient(config) - - // Verify that we fail to match the checksum - url, err := client.Get() - if err != nil { - t.Fatalf("Unexpected error: \"%v\"", err) - } - - if sourcePath != url { - t.Errorf("Inplace file get should return same path, expected %s, got %s", sourcePath, url) - } - - if _, err = os.Stat(sourcePath); err != nil { - t.Errorf("Could not stat source file: %s", sourcePath) - } -} - // SimulateFileUriDownload is a simple utility function that converts a uri // into a testable file path whilst ignoring a correct checksum match, stripping // UNC path info, and then calling stat to ensure the correct file exists. @@ -469,7 +432,7 @@ func SimulateFileUriDownload(t *testing.T, uri string) (string, error) { // This should be wrong. We want to make sure we don't delete Checksum: []byte("nope"), Hash: HashForType("sha256"), - Inplace: true, + CopyFile: false, } // go go go diff --git a/common/iso_config.go b/common/iso_config.go index aca468cd2..4f30580bc 100644 --- a/common/iso_config.go +++ b/common/iso_config.go @@ -24,7 +24,6 @@ type ISOConfig struct { TargetPath string `mapstructure:"iso_target_path"` TargetExtension string `mapstructure:"iso_target_extension"` RawSingleISOUrl string `mapstructure:"iso_url"` - Inplace bool `mapstructure:"iso_inplace"` } func (c *ISOConfig) Prepare(ctx *interpolate.Context) (warnings []string, errs []error) { diff --git a/common/step_download.go b/common/step_download.go index 9110653b2..03340b974 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -45,11 +45,6 @@ type StepDownload struct { // extension on the URL is used. Otherwise, this will be forced // on the downloaded file for every URL. Extension string - - // Inplace indicates wether to copy file - // versus using it inplace. - // Inplace can only be true when referencing local files. - Inplace bool } func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { @@ -66,7 +61,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste } } - ui.Say(fmt.Sprintf("Retrieving %s", s.Description)) + ui.Say(fmt.Sprintf("Downloading or copying %s", s.Description)) // First try to use any already downloaded file // If it fails, proceed to regular download logic @@ -94,12 +89,11 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste config := &DownloadConfig{ Url: url, TargetPath: targetPath, - Inplace: s.Inplace, + CopyFile: false, Hash: HashForType(s.ChecksumType), Checksum: checksum, UserAgent: useragent.String(), } - downloadConfigs[i] = config if match, _ := NewDownloadClient(config).VerifyChecksum(config.TargetPath); match { @@ -111,11 +105,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste if finalPath == "" { for i, url := range s.Url { - if s.Inplace { - ui.Message(fmt.Sprintf("Using file in-place: %s", url)) - } else { - ui.Message(fmt.Sprintf("Transferring file from path: %s", url)) - } + ui.Message(fmt.Sprintf("Downloading or copying: %s", url)) config := downloadConfigs[i] From a5587e30ec55e81b0ac373b50900ce73dd7ca5bc Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 20 Aug 2018 11:38:41 +0200 Subject: [PATCH 2/2] log wether the file was transfered or is just being inplace referenced --- common/step_download.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/common/step_download.go b/common/step_download.go index 03340b974..1719c5987 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -61,7 +61,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste } } - ui.Say(fmt.Sprintf("Downloading or copying %s", s.Description)) + ui.Say(fmt.Sprintf("Retrieving %s", s.Description)) // First try to use any already downloaded file // If it fails, proceed to regular download logic @@ -104,9 +104,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste } if finalPath == "" { - for i, url := range s.Url { - ui.Message(fmt.Sprintf("Downloading or copying: %s", url)) - + for i := range s.Url { config := downloadConfigs[i] path, err, retry := s.download(config, state) @@ -159,6 +157,11 @@ func (s *StepDownload) download(config *DownloadConfig, state multistep.StateBag if err != nil { return "", err, true } + if download.config.CopyFile { + ui.Message(fmt.Sprintf("Transferred: %s", config.Url)) + } else { + ui.Message(fmt.Sprintf("Using file in-place: %s", config.Url)) + } return path, nil, true case <-progressTicker.C: