Add an http test method #1403
Add an http test method #1403
Conversation
There was a problem hiding this comment.
@guilhem This is a great addition to our suite. Lets try to use native ruby classes instead of the requests gem. I propose to reuse http helper already used for the compliance plugin. We could move the helper to inspec/lib/utils https://github.com/chef/inspec/tree/master/lib/utils
| name 'http' | ||
| desc 'Use the http InSpec audit resource to test http call.' | ||
| example " | ||
| describe http(http://localhost:8080/ping', auth: ['user, 'test'], params: {format: 'html'}) do |
There was a problem hiding this comment.
maybe better:
describe http('http://localhost:8080/ping', auth: ['user, 'test'], params: {format: 'html'}) do
its('status') { should cmp 200 }
its('body') { should cmp 'pong' }
its('Content-Type') { should include('text/html') }
end
| @@ -38,4 +38,5 @@ Gem::Specification.new do |spec| | |||
| spec.add_dependency 'sslshake', '~> 1' | |||
| spec.add_dependency 'parallel', '~> 1.9' | |||
| spec.add_dependency 'rspec_junit_formatter', '~> 0.2.3' | |||
| spec.add_dependency 'requests', '~> 1.0.0' | |||
There was a problem hiding this comment.
Do we really need the requests module? It is not maintained for a couple of years anymore. See https://github.com/cyx/requests.
There was a problem hiding this comment.
@chris-rock yes it isn't well maintained but it works like a charm.
I try to use native Net::HTTP but it finally was quite the same as requests... with less stability.
I also try to find another lib without success.
requests wasn't a wonderful choice but a pragmatic one. :/
There was a problem hiding this comment.
@chris-rock or I just find http://www.rubydoc.info/gems/http/HTTP/Request https://github.com/httprb/http who is:
- quite maintained :)
- some more work to use it "as it" (basic auth etc) :|
- has some dependencies :(
It's up to you :)
| @auth = auth | ||
| @headers = headers | ||
| @data = data | ||
| @request = Requests.request(@method, @url, |
There was a problem hiding this comment.
I think we should try to use Net::HTTP. We could extract some helpers and place them into inspec utils from https://github.com/chef/inspec/blob/master/lib/bundles/inspec-compliance/http.rb
There was a problem hiding this comment.
as said in other comment, doing stuff in Net::HTTP is really painful.
https, headers, path, params, method everything is in different class without good layering.
URI / Request / ...
Requests already do the stuff. If 1 day requests stop working (with a ruby X.X) it would be time to vendor it inside our code.
For the moment it seems quite overkill to refactor inside our code a lib without any other dependency.
There was a problem hiding this comment.
I am open for another gem. Unfortunately we cannot go with the current one. Since, once merged, this PR it is part of a maintained project, we need to be sure we get all PRs in a way that we can maintain them going forward. I am open for alternative solutions.
There was a problem hiding this comment.
@chris-rock link to #1403 (comment)
is https://github.com/httprb/http ok for you?
|
@chris-rock I changed to use |
|
@guilhem I apologize for the delayed response. I'd like your change. Can you please add documentation as well in here https://github.com/chef/inspec/tree/master/docs/resources? |
|
@guilhem Anything I can help you with to get this merged this week? |
|
@chris-rock sorry I'm in holiday this week, but you can push anything you want in my branch :) |
|
@guilhem Enjoy our holiday, lets bring this in next week then. |
|
@chris-rock should be ok |
There was a problem hiding this comment.
Awesome work @guilhem I just have some minor comments. Once those are fixed, we are ready to get the resource in
| # encoding: utf-8 | ||
| # copyright: 2017, Criteo | ||
| # author: Guilhem Lettron | ||
| # license: All rights reserved |
There was a problem hiding this comment.
We do not allow All rights reserved. Every contribution needs to be Apache 2 licensed
There was a problem hiding this comment.
ok for me, I just copy/pasta other resources with same lines :)
| @data = data | ||
| http = HTTP.headers(@headers) | ||
| http = http.basic_auth(@auth) unless @auth.empty? | ||
| @response = http.request(@method, @url, { body: @data, params: @params }) |
There was a problem hiding this comment.
We should do a lazy response request, otherwise the request will be done during resource instantiation. Lets have a private method request that is called by all other functions and it should also cache the result in @response
|
@guilhem Can you rebase on latest master? functional tests are fixed there. I apologise for the inconvenience. |
Signed-off-by: Guilhem Lettron <g.lettron@criteo.com>
|
@chris-rock should be ok :) |
|
Awesome work @guilhem |
fix #336