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 helper class for creating web hooks #917

Merged
merged 8 commits into from
Nov 11, 2015

Conversation

hnrkndrssn
Copy link
Contributor

Introducing NewRepositoryWebHook, a new strongly typed class that derives from the existing NewRepositoryHook class but adds properties and constructor parameters for config settings that should be passed to the API when creating a new web hook. These settings are merged with the config settings that are specific to the web service the web hook will be interacting with (as per https://api.github.com/hooks).

Fixes #914

@haacked
Copy link
Contributor

haacked commented Sep 28, 2015

Currently there are 2 constructors, one that takes the required (name, config and url) parameters and defaults the optional (contentType, secret and insecureSsl) parameters and one that takes all parameters.

Funny you should mention this. I just submitted a PR that includes some guidelines around our model objects. https://github.com/octokit/octokit.net/blob/consolidate-committer-info/Octokit/Models/README.md

For request objects (such as this), I think optional parameters should simply be read/write properties and required parameters are read only and go in the constructor. Does that make sense?

@hnrkndrssn
Copy link
Contributor Author

👍 will update PR tonight

@hnrkndrssn hnrkndrssn force-pushed the feature-webhookhelper branch from 01ec4f6 to 8c962d8 Compare October 4, 2015 09:28
@hnrkndrssn
Copy link
Contributor Author

NewRepositoryWebHook now conforms to the model request guidelines of required properties are added as ctor parameters with read-only properties and optional properties are added as read-write properties.

Also added a method, ToRequest(), that merges the passed in config object with the webhook specific config properties that is used when passing the object to the API.

Failing test is the known StopsMakingNewRequestsWhenTakeIsFulfilled.

public string Secret { get; set; }

/// <summary>
/// Gets wether the SSL certificate of the host will be verified when
Copy link
Contributor

Choose a reason for hiding this comment

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

/wether/whether

@hnrkndrssn
Copy link
Contributor Author

We now throw an exception with a more helpful message if the user has added any webhook specific config values to the config parameter in the ctor.

Come to think of it, I need to do a bit of refactoring to clean this up a bit...

@hnrkndrssn hnrkndrssn force-pushed the feature-webhookhelper branch from d442b24 to b0ccf1a Compare October 13, 2015 07:55
@hnrkndrssn
Copy link
Contributor Author

I think this is now done, just awaiting the ✅ of approval from AppVeyor

@shiftkey
Copy link
Member

shiftkey commented Nov 4, 2015

Looks like there's a csproj merge conflict here 😢

@hnrkndrssn
Copy link
Contributor Author

crap, totally missed your comment... 😞 will try and fix it up as soon as possible.

@hnrkndrssn hnrkndrssn force-pushed the feature-webhookhelper branch from 5a6c4a5 to e643534 Compare November 11, 2015 13:07
@hnrkndrssn
Copy link
Contributor Author

OK, merge conflicts sorted, now just waiting for AppVeyor

@shiftkey
Copy link
Member

I don't think that second build is ever going to complete, so I'm gonna merge this in.

Thanks again @alfhenrik!

shiftkey added a commit that referenced this pull request Nov 11, 2015
Add helper class for creating web hooks
@shiftkey shiftkey merged commit ddcfc0a into octokit:master Nov 11, 2015
@hnrkndrssn
Copy link
Contributor Author

👍

@hnrkndrssn hnrkndrssn deleted the feature-webhookhelper branch November 11, 2015 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants