Switch to using rack-bodyparser #36

alexjfisher · 2017-11-21T17:16:46Z

No description provided.

alexjfisher · 2017-11-21T20:35:30Z

lib/helpers/data_parsers.rb

@@ -15,4 +15,8 @@ def verify_signature(payload_body)
    signature = 'sha1=' + OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), settings.github_secret, payload_body)
    throw(:halt, [500, "Signatures didn't match!\n"]) unless Rack::Utils.secure_compare(signature, request.env['HTTP_X_HUB_SIGNATURE'])
  end
+
+  def payload


I'm not sure about this. It's only very slightly more convenient than using env['parsed_body'] and there might be a better way. I'm going to see what https://github.com/aars/rack-bodyparser#patch-rackrequest is all about.

I like this. Even if it's not that much shorter, it's much clearer naming.

Cool. I did look at https://github.com/aars/rack-bodyparser#patch-rackrequest, but it didn't really seem much of an improvement. Instead of env['parsed_body'] you could use eg request.payload.

alexjfisher · 2017-11-21T20:36:35Z

lib/parsers/webhook_json_parser.rb

-        return false unless @data['repository'].key? 'html_url'
-        return false unless @data['repository']['html_url'] =~ %r{github\.com}
-        true
+        @env.key?('HTTP_X_GITHUB_EVENT')


This is the big win. It's so much nicer now that we have easy access to the headers.

Much safer too, since that's the public API. Relying on internal implementation details always seems so hacky. (not to mention that it would fail on self-hosted GitHub appliances.)

alexjfisher · 2017-11-21T20:38:22Z

spec/unit/puppet_webhook_spec.rb

@@ -6,4 +6,12 @@
    expect(last_response).to be_ok
    expect(last_response.body).to eq('{"status":"success","message":"running"}')
  end
+
+  it '/echo returns parsed payload' do


Testing the parser via a route might be easier than testing the parser class directly?

dhollinger · 2017-11-21T22:49:58Z

lib/routes/default.rb

@@ -12,6 +12,10 @@ def self.registered(puppet_webhook)
        puppet_webhook.get '/heartbeat' do
          return 200, { status: :success, message: 'running' }.to_json
        end
+
+        puppet_webhook.post '/echo' do


I'm not sure that we should have this here. The function that this endpoint is providing can already be found via Ruby development tools, the puppet_webhook log, and the spec tests.

alexjfisher · 2017-11-21T22:58:33Z

For reference. Here is my idea about how we could update the parser testing. alexjfisher@2600e1c

coveralls · 2017-11-21T22:59:49Z

Coverage decreased (-6.5%) to 40.141% when pulling cb60a69 on alexjfisher:rack_body_parser into e1557d7 on voxpupuli:master.

alexjfisher · 2017-11-21T23:19:14Z

Test coverage is down quite a bit by stubbing detect_vcs. Whatever approach we take, we should probably get it back to where it was before merging anything. I think I want to at least include the attribute accessor changes from alexjfisher@2600e1c#diff-8a56f49e6638cadc28a93357766ba2a7

P.S. Since I opened the PR, github won't let me review it (which kinda sucks). I must also remember to edit the commit message on my first commit. It's still marked WIP.

binford2k

imo, reducing coverage by removing unhelpful tests is still a net positive. I'm digging this.

alexjfisher · 2017-11-22T08:34:11Z

@binford2k I was fine with removing the /echo endpoint, but how should we test detect_vcs ?

coveralls · 2017-11-22T09:35:21Z

Coverage decreased (-6.0%) to 40.714% when pulling a41e1a0 on alexjfisher:rack_body_parser into e1557d7 on voxpupuli:master.

alexjfisher · 2017-11-22T12:02:23Z

Added slightly tweaked version of ekohl@5afb937 and rebased.

ekohl · 2017-11-22T12:05:04Z

spec/spec_helper.rb

@@ -25,6 +25,10 @@ module Methods
      def read_fixture(name)
        File.read(File.join(File.expand_path('..', __FILE__), 'fixtures', name))
      end
+
+      def read_json_fixture(name)
+        JSON.load(read_fixture(name))


Guess I should have used JSON.parse instead.

yep. Just spotted that. Commit amended and repushed.

coveralls · 2017-11-22T12:12:07Z

Coverage decreased (-1.3%) to 45.357% when pulling 46c5683 on alexjfisher:rack_body_parser into e1557d7 on voxpupuli:master.

alexjfisher · 2017-11-22T12:13:08Z

@bastelfreak Odd the codacy-bot can't delete its own comments.

ekohl

Are those codacy comments really adding something or can we disable them? You can always view the details on the website.

This was unreachable since we raise an error if we don't detect one of the supported VCSs.

coveralls · 2017-11-22T12:22:32Z

Coverage decreased (-1.2%) to 45.52% when pulling 471852f on alexjfisher:rack_body_parser into e1557d7 on voxpupuli:master.

alexjfisher · 2017-11-22T12:23:44Z

@ekohl Personally, I quite like them. I can see how they'd be useful when we have new contributors.

ekohl · 2017-11-22T12:25:12Z

lib/parsers/webhook_json_parser.rb

@@ -87,8 +87,6 @@ def deleted?
          @data['push']['changes'][0]['closed']
        when 'tfs'
          @data['resource']['refUpdates'][0]['newObjectId'] == '0000000000000000000000000000000000000000'
-        else
-          false


Why would you remove this? I actually like being able to rely on a boolean.

It was unreachable. We already raise an exception if we can't detect a VCS. I can put it back though.

alexjfisher · 2017-11-22T12:30:14Z

I think I should set something for 'COVERAGE DECREASE THRESHOLD FOR FAILURE' in coveralls though. We haven't really reduced our coverage, but we have removed code and that's given us a smaller percentage.

alexjfisher commented Nov 21, 2017

View reviewed changes

voxpupuli deleted a comment Nov 21, 2017

dhollinger suggested changes Nov 21, 2017

View reviewed changes

dhollinger requested review from ekohl and binford2k November 21, 2017 22:58

dhollinger approved these changes Nov 21, 2017

View reviewed changes

voxpupuli deleted a comment Nov 21, 2017

binford2k approved these changes Nov 21, 2017

View reviewed changes

voxpupuli deleted a comment Nov 22, 2017

ekohl approved these changes Nov 22, 2017

View reviewed changes

alexjfisher and others added 2 commits November 22, 2017 11:57

Switch to rack-bodyparser and make use of headers

4666e23

Update tests to mock results of detect_vcs

764ea49

alexjfisher force-pushed the rack_body_parser branch from a41e1a0 to 081c0a2 Compare November 22, 2017 12:00

voxpupuli deleted a comment Nov 22, 2017

ekohl suggested changes Nov 22, 2017

View reviewed changes

Test detect_vcs

46c5683

alexjfisher force-pushed the rack_body_parser branch from 081c0a2 to 46c5683 Compare November 22, 2017 12:10

voxpupuli deleted a comment Nov 22, 2017

ekohl approved these changes Nov 22, 2017

View reviewed changes

Remove default case in deleted?

471852f

This was unreachable since we raise an error if we don't detect one of the supported VCSs.

voxpupuli deleted a comment Nov 22, 2017

ekohl reviewed Nov 22, 2017

View reviewed changes

alexjfisher changed the title ~~WIP: Switch to using rack-bodyparser~~ Switch to using rack-bodyparser Nov 22, 2017

alexjfisher merged commit c14fa35 into voxpupuli:master Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using rack-bodyparser #36

Switch to using rack-bodyparser #36

alexjfisher commented Nov 21, 2017

alexjfisher Nov 21, 2017

ekohl Nov 22, 2017

alexjfisher Nov 22, 2017

alexjfisher Nov 21, 2017

binford2k Nov 21, 2017

alexjfisher Nov 21, 2017

dhollinger Nov 21, 2017

alexjfisher commented Nov 21, 2017

coveralls commented Nov 21, 2017

alexjfisher commented Nov 21, 2017 •

edited

Loading

binford2k left a comment

alexjfisher commented Nov 22, 2017

coveralls commented Nov 22, 2017

alexjfisher commented Nov 22, 2017

ekohl Nov 22, 2017

alexjfisher Nov 22, 2017

coveralls commented Nov 22, 2017

alexjfisher commented Nov 22, 2017

ekohl left a comment

coveralls commented Nov 22, 2017

alexjfisher commented Nov 22, 2017

ekohl Nov 22, 2017

alexjfisher Nov 22, 2017

alexjfisher commented Nov 22, 2017

Switch to using rack-bodyparser #36

Switch to using rack-bodyparser #36

Conversation

alexjfisher commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher commented Nov 21, 2017

coveralls commented Nov 21, 2017

alexjfisher commented Nov 21, 2017 • edited Loading

binford2k left a comment

Choose a reason for hiding this comment

alexjfisher commented Nov 22, 2017

coveralls commented Nov 22, 2017

alexjfisher commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 22, 2017

alexjfisher commented Nov 22, 2017

ekohl left a comment

Choose a reason for hiding this comment

coveralls commented Nov 22, 2017

alexjfisher commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher commented Nov 22, 2017

alexjfisher commented Nov 21, 2017 •

edited

Loading