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

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

Merged

Conversation

didil
Copy link
Contributor

@didil didil commented Feb 5, 2019

🎟️ 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
Copy link

codecov bot commented Feb 5, 2019

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.

Copy link
Member

@bobheadxi bobheadxi left a comment

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
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
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
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
Copy link
Contributor

@seifghazi seifghazi left a comment

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
Member

@didil can you also revert c3ed6e2, as with this PR the warning no longer applies πŸ˜„ thanks!

@didil
Copy link
Contributor Author

didil commented Feb 6, 2019

changes pushed

Copy link
Member

@bobheadxi bobheadxi left a comment

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