Add support for downloading puppet URL’s #270
Conversation
bastelfreak
added
enhancement
|
Hi @hajee, thanks for the great PR! do you think you can also add some tests? |
| @@ -117,28 +117,16 @@ archive { '/tmp/test100k.db': | |||
|
|
|||
| ### Puppet URL | |||
|
|
|||
| Below is an example of how to deploy a tar.gz to a local directory when it is | |||
| served via the ```puppet:///``` style url which is not currently supported. | |||
| Since march 2017, the Archive type also supports puppet url's. Here is an example | |||
There was a problem hiding this comment.
I'm guessing that ruby is the only provider that will support this feature? It's not the default provider on Linux. Does the example need a provider => ruby?
There was a problem hiding this comment.
I was expecting the default provider to be ruby, but I was wrong. I will update the readme
There was a problem hiding this comment.
@hajee Actually, you might be ok. Ruby isn't the default provider, but it is still the parent provider for curl and wget providers which override the parent's download method. Could you double-check? For example, looks like things might still go wonky if a user tries to specify a checksum_url.
There was a problem hiding this comment.
So not sure what happens if the puppet:/// url points to a directory v.s. a file. Maybe we should clarify it in documentation about expectations if it doesn't handle directory.
There was a problem hiding this comment.
@nanliu : If you mean a puppet url pointing to a directory when using puppet apply, it does handle that.
lib/puppet/type/archive.rb
Outdated
| validate do |value| | ||
| unless value =~ URI.regexp(%w(http https ftp file s3)) || Puppet::Util.absolute_path?(value) | ||
| unless value =~ URI.regexp(%w(puppet http https ftp file s3)) || Puppet::Util.absolute_path?(value) | ||
| raise ArgumentError, "invalid source url: #{value}" | ||
| end |
There was a problem hiding this comment.
@hajee Perhaps you could declare a provider feature? Something like...
feature :puppet_urls, 'The ability to download from Puppet URLs' if value == 'puppet'and then in the ruby provider
has_feature :puppet_urlsThere was a problem hiding this comment.
Good suggestion. Just wondering: is it possible to do a different validate for the provider too?
There was a problem hiding this comment.
Don't need this for now since everything inherits from the ruby provider. feature :puppet_urls is only necessary if we start to differentiate features per provider.
There was a problem hiding this comment.
Follow up with chat with @alexjfisher, it might be worthwhile to mention in README, puppet:/// source will use ruby provider regardless of provider setting for now. But not a blocker for merging.
lib/puppet/type/archive.rb
Outdated
| validate do |value| | ||
| unless value =~ URI.regexp(%w(http https ftp file s3)) || Puppet::Util.absolute_path?(value) | ||
| unless value =~ URI.regexp(%w(puppet http https ftp file s3)) || Puppet::Util.absolute_path?(value) | ||
| raise ArgumentError, "invalid source url: #{value}" | ||
| end |
There was a problem hiding this comment.
Don't need this for now since everything inherits from the ruby provider. feature :puppet_urls is only necessary if we start to differentiate features per provider.
| @@ -117,28 +117,16 @@ archive { '/tmp/test100k.db': | |||
|
|
|||
| ### Puppet URL | |||
|
|
|||
| Below is an example of how to deploy a tar.gz to a local directory when it is | |||
| served via the ```puppet:///``` style url which is not currently supported. | |||
| Since march 2017, the Archive type also supports puppet url's. Here is an example | |||
There was a problem hiding this comment.
So not sure what happens if the puppet:/// url points to a directory v.s. a file. Maybe we should clarify it in documentation about expectations if it doesn't handle directory.
|
Last note, if we want to be smart about conditional downloads, we need to implement checksum retrieval as well via http://www.rubydoc.info/gems/puppet/Puppet/FileServing/Metadata checksum and checksum_type. There isn't a clean way to do this because I'm not aware of way to change default provider based on a parameter. If we could change default provider on the fly based on source url, it will make the implementation much cleaner, instead we need to settle with cramming everything into the ruby provider (instead of ruby/s3/puppet providers). |
|
Hi guys. Thanks for all the feedback. What are the things that still need attention before this PR can be merged? |
|
@hajee Just a note in the README explaining that the ruby provider will be used regardless of provider. I think we were then good to go. |
|
@hajee Could you rebase? @bastelfreak Are you happy to remove the 'needs-tests' label now? |
|
@alexjfisher rebase executed |
|
@alexjfisher @bastelfreak Is there an ATA on the merge. I'd hate to do a rebase again :-) |
|
Thanks for the PR @hajee! |
|
Thanks for all the reviews and suggestions. |
This PR allows archive to support puppet urls for the source parameter.