Improvement to $destination param (issue #51)

[LinguineCode]

This is for Issue #51. First attempt was PR #56, which was closed because owner of repo wanted it improved further. I'll quote the original PR below.

This modification allows you to iterate over an array of URLs and wget them all to one directory. This currently has to be done with 4 separate declarations of
wget::fetch { 'http://url': destination => '/path' }

  $manyfiles = [
    'https://www.kernel.org/pub/linux/kernel/v1.0/linux-1.0.tar.gz',
    'https://www.kernel.org/pub/linux/kernel/v1.0/linux-1.0.tar.sign',
    'https://www.kernel.org/pub/linux/kernel/v1.0/linux-1.0.tar.xz',
  ]
  wget::fetch { $manyfiles:
    destination => '/tmp/',
  }

And another way it has improved: Given the most simple of example of using your module, just downloading a single file, you no longer have to specify the filename. It just takes the file's name of the URL. Looks a lot cleaner imho.

wget::fetch { 'https://www.kernel.org/pub/linux/kernel/v1.0/linux-1.0.tar.bz2':
    destination_dir => '/tmp/',
  }

Read PR #56 for more detail on why the original implementation did not work.

To make it work, owner originally requested the improvement be: No new params for this feature, so make it if $destination already exists as a directory, then treat it as a directory. But checking to see if a directory exists is difficult to do with Puppet (and impossible to do cleanly).

So I settled on the following design which looks and works great. Better than the original PR too imho: If $destination ends in a forward or backward slash, then treat as a directory. With this, we were able to achieve:

1. Backwards compatibility with older versions
2. No new parameters have been added

Old style still works too:

  wget::fetch { 'https://www.kernel.org/pub/linux/kernel/v1.0/linux-1.0.tar.bz2':
    destination => '/tmp/something.bz2',
  }

[carlossg]

awesome, LGTM
if you could just add a test to spec/defines/fetch_spec.rb to ensure it behaves as expected

[LinguineCode]

I haven't learned spec tests yet. I made an attempt but now checks are failing. Can you give me some guidance? I was attempting to:

1. change the "default" $destination param from /tmp/dest to /tmp/dest/ (trailing slash)
2. Add a new spec test to make sure that if $destination => /tmp/something_specific.txt, the wget command should contain --output-document="/tmp/something_specific.txt"

[LinguineCode]

That first attempt at rewriting the spec test was horrible. What was I thinking?

I'm gonna try it again. Will update this PR within a few days

[carlossg]

you probably don't want to change existing tests, add one more or less like

  context "download to dir", :compile do
    let(:params) { super().merge({
      :destination => '/tmp/dest/',
    })}

    it { should contain_exec('wget-test').with({
      'command' => "wget --no-verbose --output-document=\"#{destination}source\" \"http://localhost/source\"",
      'environment' => []
    }) }
  end

[LinguineCode]

Got it, will do

[LinguineCode]

@carlossg that spec test looked OK to me but it didn't work. i'm gonna keep at it, but if you have any advice let me know

[LinguineCode]

OK, test added. We are good to go

[LinguineCode]

thanks @carlossg. on second look, the README.md that I modified now looks unorganized. I'll send another PR to improve it.