Skip to content

Add warning that GitHub webhook Content-Type must be JSON#445

Merged
bobheadxi merged 1 commit into
ubclaunchpad:masterfrom
rwblickhan:rwblickhan/#444-no-notification-of-json-github-webhook
Nov 8, 2018
Merged

Add warning that GitHub webhook Content-Type must be JSON#445
bobheadxi merged 1 commit into
ubclaunchpad:masterfrom
rwblickhan:rwblickhan/#444-no-notification-of-json-github-webhook

Conversation

@rwblickhan

Copy link
Copy Markdown
Contributor

🎟️ Ticket(s): Closes #444


👷 Changes

Adds a brief warning that Content-Type for GitHub webhook must be JSON after providing it.

@rwblickhan rwblickhan requested a review from bobheadxi as a code owner November 7, 2018 23:19
Comment thread client/client.go Outdated
fmt.Fprint(c.out, "\n"+`Note that you will have to disable SSH verification in your webhook
settings - Inertia uses self-signed certificates that GitHub won't
be able to verify.`+"\n")
fmt.Fprint(c.out, "\n"+`Note that you will have to set the Content-Type for your webhook

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.

why not put this together with the previous note?

@rwblickhan rwblickhan Nov 7, 2018

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.

No particular reason, though I imagine those two lines might be changed independently in the future, so it's worth having two separate messages - on the other hand I can see the argument that it's worth putting them together so people actually read both.

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.

thats fair, though for the sake of smoothness maybe Note that you will *also* have to set... would flow better 😋

Also linters are complaining about spacing 😢

https://travis-ci.org/ubclaunchpad/inertia/jobs/452143344#L527

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.

Yeah, that's fair, can update

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.

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.

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.

This makes me extremely sad. 😞

But I can fix it.

@bobheadxi bobheadxi added the pr: finalized needs review and final approval label Nov 7, 2018
@rwblickhan rwblickhan force-pushed the rwblickhan/#444-no-notification-of-json-github-webhook branch from 9317cd7 to 994db75 Compare November 7, 2018 23:40
@codecov

codecov Bot commented Nov 7, 2018

Copy link
Copy Markdown

Codecov Report

Merging #445 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage    45.2%   45.24%   +0.05%     
==========================================
  Files          67       67              
  Lines        3525     3528       +3     
==========================================
+ Hits         1593     1596       +3     
  Misses       1747     1747              
  Partials      185      185
Impacted Files Coverage Δ
client/client.go 72.97% <100%> (+0.36%) ⬆️

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 243bcb8...994db75. Read the comment docs.

@bobheadxi bobheadxi merged commit c3ed6e2 into ubclaunchpad:master Nov 8, 2018
didil added a commit to didil/inertia that referenced this pull request Feb 6, 2019
bobheadxi pushed a commit that referenced this pull request Feb 6, 2019
* add support for GitHub form-encoded webhooks

* Revert "Add warning that GitHub webhook Content-Type must be json (#445)" (c3ed6e2)
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.

2 participants