Skip to content

daemon: added support for github form-encoded webhooks#528

Merged
bobheadxi merged 4 commits into
ubclaunchpad:masterfrom
didil:daemon/#369-github-form-encoded-webhooks
Feb 6, 2019
Merged

daemon: added support for github form-encoded webhooks#528
bobheadxi merged 4 commits into
ubclaunchpad:masterfrom
didil:daemon/#369-github-form-encoded-webhooks

Conversation

@didil

@didil didil commented Feb 5, 2019

Copy link
Copy Markdown
Contributor

🎟️ Ticket(s): Closes #369


👷 Changes

Added content-type application/x-www-form-urlencoded support for github
github request form body:
payload={the-same-json-payload-from-json-requests}

🔦 Testing Instructions

make tests
local daemon and test project / webhooks

@didil didil requested a review from bobheadxi as a code owner February 5, 2019 09:40
@codecov

codecov Bot commented Feb 5, 2019

Copy link
Copy Markdown

Codecov Report

Merging #528 into master will increase coverage by 0.14%.
The diff coverage is 65.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   56.09%   56.22%   +0.14%     
==========================================
  Files          60       60              
  Lines        2951     2983      +32     
==========================================
+ Hits         1655     1677      +22     
- Misses       1089     1098       +9     
- Partials      207      208       +1
Impacted Files Coverage Δ
client/client.go 70.47% <ø> (-0.36%) ⬇️
daemon/inertiad/webhook/github.go 64.71% <63.64%> (-1.96%) ⬇️
daemon/inertiad/webhook/bitbucket.go 66.67% <66.67%> (ø) ⬆️
daemon/inertiad/webhook/parse.go 76.93% <66.67%> (+1.93%) ⬆️
daemon/inertiad/webhook/gitlab.go 66.67% <66.67%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d45be4...09ca84c. Read the comment docs.

@bobheadxi bobheadxi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is terrific, thanks @didil !

Just a few changes before we can merge this:

  • can you remove the changes to .static? I notice that there's something weird going on with the images for some people, but I'd rather it be resolved in a separate PR
  • I notice you've changed the permissions on test/keys/id_rsa from 755 to 644 - it was recently changed to 755 in 5a0414e, since one of our members (@mRabitsky ) was having issues with his SSH client saying that the permissions on the key were too open. Did the key not work for you as-is? If it did, can you revert the change? If it did not, can you revert the change anyway? I'll open up a ticket to look into how to resolve this long-term

Thank you!

@bobheadxi bobheadxi added the pr: finalized needs review and final approval label Feb 5, 2019
terryz21
terryz21 previously approved these changes Feb 5, 2019
Comment thread daemon/inertiad/webhook/github.go Outdated
case "application/json":
return body, nil
default:
return nil, errors.New("Github Webhook Content-Type must be application/json or x-www-form-urlencoded")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Github webhook Content-Type must be application/json or application/x-www-form-urlencoded" 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done thanks !

seifghazi
seifghazi previously approved these changes Feb 5, 2019

@seifghazi seifghazi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💯

nicholaschinjie
nicholaschinjie previously approved these changes Feb 5, 2019
@bobheadxi

Copy link
Copy Markdown
Member

@didil can you also revert c3ed6e2, as with this PR the warning no longer applies 😄 thanks!

@didil didil dismissed stale reviews from nicholaschinjie, seifghazi, and terryz21 via 09ca84c February 6, 2019 01:58
@didil

didil commented Feb 6, 2019

Copy link
Copy Markdown
Contributor Author

changes pushed

@bobheadxi bobheadxi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @didil ! This is great 🚀

@bobheadxi bobheadxi merged commit 9a04c0b into ubclaunchpad:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: finalized needs review and final approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants