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

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

Conversation

rwblickhan
Copy link
Contributor

🎟️ Ticket(s): Closes #444


👷 Changes

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

client/client.go Outdated
@@ -122,6 +122,9 @@ func (c *Client) BootstrapRemote(repoName string) error {
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
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?

Copy link
Contributor Author

@rwblickhan rwblickhan Nov 7, 2018

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
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
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
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

codecov bot commented Nov 7, 2018

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