Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added wget provider when using unix. #61

Merged
merged 7 commits into from
May 29, 2015
Merged

Added wget provider when using unix. #61

merged 7 commits into from
May 29, 2015

Conversation

hajee
Copy link
Contributor

@hajee hajee commented May 27, 2015

Also extracted rspec's for archive providers. Righ now there is no specific test for either the wget provider, so it uses the normal one.

Renamed the default provider to ruby, because now we have more then one provider and this name is a better fit for what it's doing.

This PR closes #60 at least for unix systems containing wget.

Also extracted rspec's for archive providers. Righ now there is no specific test for either the wget provider, so it uses the normal one.

Renamed the default provider to ruby, because now we have more then one provider and this name is a better fit for what it's doing.
@nanliu
Copy link
Contributor

nanliu commented May 27, 2015

Overall looks fine, we still need to migrate require 'puppet_x/bodeco/util' so it doesn't load faraday gem. I'm not sure we need shared context since as a parent provider methods don't need to be tested multiple times. If we implement spec it would be strictly for the download method for the wget provider.

Puppet::Type.type(:archive).provide(:wget, :parent => :ruby ) do

commands :wget => '/usr/bin/wget'
defaultfor :operatingsystem => :linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe operatingsystem fact returns linux. This repo is pretty out of date, but would give a better idea what facts might be suitable:
https://github.com/apenney/puppet_facts/blob/master/PE2.8/redhat-5-i386.facts#L52

I believe the closest might be feature_match, which would be :feature => :posix:
https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider.rb#L285

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operatingsystem fact indeed doesn't return :linux. The strange thing is, I testen in on a linux system and it works. I could also check if it is not windows. What do you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it just used the default ruby provider when you were testing. I don't see a way to test for negate in defaultfor, only for confine in the puppet provider code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used puppet --debug and saw the command. Also used the debugger to actually step though the code. Anyway.. I'll change it to:

defaultfor :feature => :posix

or

@nanliu
Copy link
Contributor

nanliu commented May 27, 2015

If we switch default provider for linux, not sure if curl or wget be the default provider, or should we implement it as a fact (similar to below)?

https://github.com/puppet-community/puppet-staging/blob/master/lib/facter/staging_http_get.rb

@hajee
Copy link
Contributor Author

hajee commented May 27, 2015

My reasoning was that if wget is not available, we fall back to the default implementation in ruby.

@hajee
Copy link
Contributor Author

hajee commented May 27, 2015

The code right now still needs the faraday gem when using the ruby provider. My reasons for using the shared behaviour was based on the idea that you could have more providers with all slightly different behaviour.

If you think it is YAGNU, I'll remove it.

@nanliu nanliu mentioned this pull request May 27, 2015
5 tasks
@nanliu
Copy link
Contributor

nanliu commented May 27, 2015

So content and download are the only method dependent on faraday, I think we can make net::http the default provider if we remove redirect as a requirement and make faraday another provider. This should allow migration:

Archive {
  provider => 'faraday' 
}

I've listed the goals for 0.4.0 in #62.

Bert Hajee added 6 commits May 28, 2015 11:16
No more deprecation messages when running the specs
This is the base for realising a ruby provider without faraday. But use the faraday as a fallback. It also removed the `feature => faraday` for the other providers
@hajee
Copy link
Contributor Author

hajee commented May 28, 2015

I've finished the request changes and some more. I noticed that wget doesn't handle file: urls. That was my use case so therefore I also added a curl provider.

In #62 you listed the faraday provider. So I extracted the faraday specific stuff from the ruby provider into a faraday provider. To get a full functional ruby provider without faraday, you would have to build a robust download method. I'll leave that to someone else ;-)

Although both providers have a unit spec, the whole type and provider, could use a Beaker acceptance test. I guess this is an other thing for the roadmap.

@nanliu
Copy link
Contributor

nanliu commented May 29, 2015

We still need to migrate the content method and implement a naive net/http download for ruby. I'll merge this as is.

nanliu added a commit that referenced this pull request May 29, 2015
Added wget provider when using unix.
@nanliu nanliu merged commit 61559df into voxpupuli:master May 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the dependency on Faraday
2 participants