From 82e480a2859868c33187c70571453f184dfcde46 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 15 Aug 2018 14:37:38 +0200 Subject: [PATCH 1/7] allow to use ISO images inplace v.s. copying them --- builder/virtualbox/iso/builder.go | 1 + builder/vmware/iso/builder.go | 1 + common/download.go | 5 +---- common/iso_config.go | 1 + common/step_download.go | 7 ++++++- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/builder/virtualbox/iso/builder.go b/builder/virtualbox/iso/builder.go index 2ea19e70f..4e906e1cd 100644 --- a/builder/virtualbox/iso/builder.go +++ b/builder/virtualbox/iso/builder.go @@ -204,6 +204,7 @@ 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.InplaceISO, }, &vboxcommon.StepOutputDir{ Force: b.config.PackerForce, diff --git a/builder/vmware/iso/builder.go b/builder/vmware/iso/builder.go index 0944db927..a884822b2 100644 --- a/builder/vmware/iso/builder.go +++ b/builder/vmware/iso/builder.go @@ -291,6 +291,7 @@ 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.InplaceISO, }, &vmwcommon.StepOutputDir{ Force: b.config.PackerForce, diff --git a/common/download.go b/common/download.go index 09457ef0b..33fa7fed9 100644 --- a/common/download.go +++ b/common/download.go @@ -154,10 +154,7 @@ func (d *DownloadClient) Get() (string, error) { return "", fmt.Errorf("Unable to treat uri scheme %s as a Downloader. : %T", u.Scheme, d.downloader) } - local, ok := d.downloader.(LocalDownloader) - if !ok && !d.config.CopyFile { - d.config.CopyFile = true - } + local, _ := d.downloader.(LocalDownloader) // If we're copying the file, then just use the actual downloader if d.config.CopyFile { diff --git a/common/iso_config.go b/common/iso_config.go index 4f30580bc..e9a784266 100644 --- a/common/iso_config.go +++ b/common/iso_config.go @@ -24,6 +24,7 @@ type ISOConfig struct { TargetPath string `mapstructure:"iso_target_path"` TargetExtension string `mapstructure:"iso_target_extension"` RawSingleISOUrl string `mapstructure:"iso_url"` + InplaceISO 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 03340b974..c35e9abef 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -45,6 +45,11 @@ 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 is only used when referencing local ISO files. + Inplace bool } func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { @@ -89,7 +94,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste config := &DownloadConfig{ Url: url, TargetPath: targetPath, - CopyFile: false, + CopyFile: !s.Inplace, Hash: HashForType(s.ChecksumType), Checksum: checksum, UserAgent: useragent.String(), From 863222b1e28876285c1d0584dccb2c195cbd44df Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 15 Aug 2018 15:26:31 +0200 Subject: [PATCH 2/7] Also use the terminology Inplace in DownloadConfig for clarity/consistency * swapped boolean checks * swapped in tests too --- common/download.go | 17 ++++++++++------- common/download_test.go | 11 ++--------- common/step_download.go | 4 ++-- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/common/download.go b/common/download.go index 33fa7fed9..84776bee9 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 - // 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 + // Inplace indicates wether to copy file + // versus using it inplace. + // Inplace can only be true when referencing local files. + Inplace bool // The hashing implementation to use to checksum the downloaded file. Hash hash.Hash @@ -154,10 +154,13 @@ func (d *DownloadClient) Get() (string, error) { return "", fmt.Errorf("Unable to treat uri scheme %s as a Downloader. : %T", u.Scheme, d.downloader) } - local, _ := d.downloader.(LocalDownloader) + local, ok := d.downloader.(LocalDownloader) + if !ok && d.config.Inplace { + d.config.Inplace = false + } // If we're copying the file, then just use the actual downloader - if d.config.CopyFile { + if d.config.Inplace == false { var f *os.File finalPath = d.config.TargetPath @@ -189,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.CopyFile { + if d.config.Inplace == false { os.Remove(finalPath) } diff --git a/common/download_test.go b/common/download_test.go index c9175b013..835b2d54b 100644 --- a/common/download_test.go +++ b/common/download_test.go @@ -58,7 +58,6 @@ func TestDownloadClient_basic(t *testing.T) { client := NewDownloadClient(&DownloadConfig{ Url: ts.URL + "/basic.txt", TargetPath: tf.Name(), - CopyFile: true, }) path, err := client.Get() @@ -94,7 +93,6 @@ func TestDownloadClient_checksumBad(t *testing.T) { TargetPath: tf.Name(), Hash: HashForType("md5"), Checksum: checksum, - CopyFile: true, }) if _, err := client.Get(); err == nil { @@ -120,7 +118,6 @@ func TestDownloadClient_checksumGood(t *testing.T) { TargetPath: tf.Name(), Hash: HashForType("md5"), Checksum: checksum, - CopyFile: true, }) path, err := client.Get() @@ -152,7 +149,6 @@ 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 { @@ -210,7 +206,6 @@ func TestDownloadClient_resume(t *testing.T) { client := NewDownloadClient(&DownloadConfig{ Url: ts.URL, TargetPath: tf.Name(), - CopyFile: true, }) path, err := client.Get() @@ -270,7 +265,6 @@ func TestDownloadClient_usesDefaultUserAgent(t *testing.T) { config := &DownloadConfig{ Url: server.URL, TargetPath: tf.Name(), - CopyFile: true, } client := NewDownloadClient(config) @@ -303,7 +297,6 @@ func TestDownloadClient_setsUserAgent(t *testing.T) { Url: server.URL, TargetPath: tf.Name(), UserAgent: "fancy user agent", - CopyFile: true, } client := NewDownloadClient(config) @@ -402,7 +395,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"), - CopyFile: false, + Inplace: true, } client := NewDownloadClient(config) @@ -432,7 +425,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"), - CopyFile: false, + Inplace: true, } // go go go diff --git a/common/step_download.go b/common/step_download.go index c35e9abef..6d2b850a0 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -48,7 +48,7 @@ type StepDownload struct { // Inplace indicates wether to copy file // versus using it inplace. - // Inplace is only used when referencing local ISO files. + // Inplace can only be true when referencing local files. Inplace bool } @@ -94,7 +94,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste config := &DownloadConfig{ Url: url, TargetPath: targetPath, - CopyFile: !s.Inplace, + Inplace: s.Inplace, Hash: HashForType(s.ChecksumType), Checksum: checksum, UserAgent: useragent.String(), From 17f2949e369bbf5cb5c6039b7c86983235881500 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 15 Aug 2018 15:51:25 +0200 Subject: [PATCH 3/7] remove stuttering; ISOConfig.InplaceISO -> Inplace --- builder/virtualbox/iso/builder.go | 2 +- builder/vmware/iso/builder.go | 2 +- common/iso_config.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builder/virtualbox/iso/builder.go b/builder/virtualbox/iso/builder.go index 4e906e1cd..7cf3dd7e3 100644 --- a/builder/virtualbox/iso/builder.go +++ b/builder/virtualbox/iso/builder.go @@ -204,7 +204,7 @@ 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.InplaceISO, + 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 a884822b2..d428d0184 100644 --- a/builder/vmware/iso/builder.go +++ b/builder/vmware/iso/builder.go @@ -291,7 +291,7 @@ 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.InplaceISO, + Inplace: b.config.Inplace, }, &vmwcommon.StepOutputDir{ Force: b.config.PackerForce, diff --git a/common/iso_config.go b/common/iso_config.go index e9a784266..aca468cd2 100644 --- a/common/iso_config.go +++ b/common/iso_config.go @@ -24,7 +24,7 @@ type ISOConfig struct { TargetPath string `mapstructure:"iso_target_path"` TargetExtension string `mapstructure:"iso_target_extension"` RawSingleISOUrl string `mapstructure:"iso_url"` - InplaceISO bool `mapstructure:"iso_inplace"` + Inplace bool `mapstructure:"iso_inplace"` } func (c *ISOConfig) Prepare(ctx *interpolate.Context) (warnings []string, errs []error) { From fae3db4e5879e868db526fc515d8167fcfc55fa8 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 15 Aug 2018 17:09:39 +0200 Subject: [PATCH 4/7] test inplace linking --- common/download_test.go | 44 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/common/download_test.go b/common/download_test.go index 835b2d54b..9305e7db8 100644 --- a/common/download_test.go +++ b/common/download_test.go @@ -411,6 +411,50 @@ 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. From 2b3ea29970dcdb45dca636c9c5c5a79a33608331 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 16 Aug 2018 18:41:44 +0200 Subject: [PATCH 5/7] show more precise error download/copy/referencing messages --- common/step_download.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/common/step_download.go b/common/step_download.go index 6d2b850a0..13f5e75fa 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "fmt" "log" + "strings" "time" "github.com/hashicorp/packer/helper/multistep" @@ -66,7 +67,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("Downloading, copying or inplace referencing %s", s.Description)) // First try to use any already downloaded file // If it fails, proceed to regular download logic @@ -99,6 +100,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste Checksum: checksum, UserAgent: useragent.String(), } + downloadConfigs[i] = config if match, _ := NewDownloadClient(config).VerifyChecksum(config.TargetPath); match { @@ -110,7 +112,11 @@ 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)) + if strings.HasPrefix(url, "file://") { + ui.Message(fmt.Sprintf("Copying or inplace referencing: %s", url)) + } else { + ui.Message(fmt.Sprintf("Downloading: %s", url)) + } config := downloadConfigs[i] From d7d4aed51c17dcc1609af633521ad4e58c58567a Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Thu, 16 Aug 2018 18:56:28 +0200 Subject: [PATCH 6/7] be even more precise --- common/step_download.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/step_download.go b/common/step_download.go index 13f5e75fa..071f2d44f 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -113,7 +113,11 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste if finalPath == "" { for i, url := range s.Url { if strings.HasPrefix(url, "file://") { - ui.Message(fmt.Sprintf("Copying or inplace referencing: %s", url)) + if s.Inplace { + ui.Message(fmt.Sprintf("Inplace referencing: %s", url)) + } else { + ui.Message(fmt.Sprintf("Copying: %s", url)) + } } else { ui.Message(fmt.Sprintf("Downloading: %s", url)) } From c744e8b2bbaf2d752bfb1536f41b17f774f5d064 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 17 Aug 2018 09:29:39 +0200 Subject: [PATCH 7/7] make download messages less redudant and more simple --- common/step_download.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/common/step_download.go b/common/step_download.go index 071f2d44f..9110653b2 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -6,7 +6,6 @@ import ( "encoding/hex" "fmt" "log" - "strings" "time" "github.com/hashicorp/packer/helper/multistep" @@ -67,7 +66,7 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste } } - ui.Say(fmt.Sprintf("Downloading, copying or inplace referencing %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 @@ -112,14 +111,10 @@ func (s *StepDownload) Run(_ context.Context, state multistep.StateBag) multiste if finalPath == "" { for i, url := range s.Url { - if strings.HasPrefix(url, "file://") { - if s.Inplace { - ui.Message(fmt.Sprintf("Inplace referencing: %s", url)) - } else { - ui.Message(fmt.Sprintf("Copying: %s", url)) - } + if s.Inplace { + ui.Message(fmt.Sprintf("Using file in-place: %s", url)) } else { - ui.Message(fmt.Sprintf("Downloading: %s", url)) + ui.Message(fmt.Sprintf("Transferring file from path: %s", url)) } config := downloadConfigs[i]