Org webhooks #1028 #1144

nubby109 · 2016-03-12T17:19:07Z

#1028 Implemented the Organization Webhook. Added the tests as well. Updated the urls of of repository hooks in some places. Added a ensure condition to check for hookid(added Please Review in comment. Please check if this should be there)

Added the organization webhooks and tests. Corrected a few repository url (related to hooks). Added a ensure condition for hookid in repository hooks

nubby109 · 2016-03-12T17:28:41Z

6 integration tests I made are failing(All in OrganizationHooksClientTests.cs). I think this is because authenticated user has to be an admin of the organization being managed and OAuth tokens require the admin:org_hook scope.
Need some help, i can't figure out how to go about it.
EDIT- Octokit.Tests tests I added are passing

ryangribble · 2016-03-13T03:09:22Z

Hey @Anubhav10 thanks heaps for the contribution! Ive done a first pass review and made several comments... Hopefully you don't think Im being overly picky! This is a fantastic contribution and in general you've nailed all the key elements - eg standard and observable implementations, great unit and integration tests, added any new files to all the csproj projects and so on.

In terms of the feedback ive commented - most are around tidy up things like whitespace and I also highlighted a couple of things around consistency in naming/doc - eg http vs https on API doc links, or using organizationName rather than org for the parameters. I realize that there are already some instances of inconsistencies within octokit in these areas but I guess my approach is when adding new work to try to conform with the "majority" consistency, and then hopefully we will also start to tackle the minority items that are inconsistent. You also seem to have made a couple of inadvertent edits to the csproj files around nuget package version and app.config binding redirects etc.

To address a couple of specific things:

Your comment about integration tests not running - it looks like you're setting "octokit" org in the test fixture, whereas you should be using a specific integration testing organization that you setup for yourself (accessed via the Helper class). I made a more detailed comment in the location where this was being done
With unit tests, when there are parameters that are being checked for empty strings as well as null (eg Ensure.ArgumentNotNullOrEmptyString(organizationName)) you should add a unit test that verifies that. So in your case, you've added the EnsuresNonNullArguments() unit test, but you could also add a EnsuresNonEmptyArguments() test that checks the exceptions are thrown when "" are passed in for the various arguments that shouldn't be allowed (such as the org name). If you search the code base, there should be some other examples of that if you need to see.
Your [PleaseReview] comments about NotNull assertions on HookId are not required, as hookId being an int can never be null 😀

nubby109 · 2016-03-13T03:22:15Z

Thanks for the review. I'll make these changes as soon as possible. It may take a few days as I have my mid-sem exams in the next week.

ryangribble · 2016-03-13T11:18:08Z

@shiftkey neither appveyor or travis builds seemed to fire on this one? The fact this is a change about organization hooks and had the "octokit" organisation mentioned in integration tests, hopefully is just a co-incidence! 😀

shiftkey · 2016-03-14T00:27:41Z

@ryangribble uh, that's, neat! Will have a look....

shiftkey · 2016-03-14T00:39:11Z

@ryangribble I was able to re-send the webhook to AppVeyor but the Travis integration doesn't have that option. Oh well, it should trigger when the PR is next synced...

…ebhooks

2. Added EnsuresNonEmptyArguments() in OrganizationHooksClientTest.cs 3. Fixed the integrationTests I added for org webhooks 4. Changes asked by ryanribble

nubby109 · 2016-03-22T06:09:07Z

Sorry for the delay. I have made the changes

Changed organizationName to org
Added EnsuresNonEmptyArguments() in OrganizationHooksClientTest.cs
Fixed the problems in integrationTests I added for org webhooks after adding the test account
And other changes as asked by @ryangribble
About the csproj and app.config changes, there was a problem in the test runner in my VS. It wasn't showing any tests. So, I updated all the Nuget Packages(It was a solution on stackoverflow), it still didn't work. I guess the update made those changes. There was a merge conflict when I commiting this time in the csproj, I have replaced these files with the one in master.

There are references to the documentation in https. Do you want me to change them all?

The integration tests have been skipped for OrganizationMemberClientTest, and the reasons given are "Admin/Member filter doesn't work unless the requester is an organization member" ,"TwoFactor filter can't be used unless the requester is an organization owner". These problems will come in org webhooks too, unless one is the admin of the org the tests will fail, so should I skip them?

One Of the integration test is failing. Test Name: Octokit.Tests.Integration.Clients.OrganizationHooksClientTests+TheCreateMethod.CreateAWebHookForTestOrganization. The reason it is failing is that the 'active' value I send is false, but in the created hook active value is true. I guess this is a problem in the GitHub API.
Also the value of secret is returned as 'HMAC hex digest of the body, using the secret as the key' I guess( there are ******) instead of the secret, so I have commented the assert to check the value of config, and remove the secret for now. This test will pass if we change the active to true in assert check, but that won't be right!! 😀

nubby109 · 2016-03-22T06:13:12Z

The travis-ci build is passing this time!! 😀

M-Zuber · 2016-03-22T10:32:10Z

Octokit.Reactive/Clients/IObservableOrganizationsClient.cs

+        /// A client for GitHub's Organization Hooks API.
+        /// </summary>
+        /// <remarks>See <a href="http://developer.github.com/v3/orgs/hooks/">Hooks API documentation</a> for more information.</remarks>
+        IObservableOrganizationHooksClient Hooks { get; }


This should be in the singular - Hook to be consistent with the other sub-client properties

I added these properties based on the iobservablerepositoriesclient, it was 'Hooks' there. I thought that is how it should be named here too. I'll change it if you want

@ryangribble is the master of consistency around here, but I believe that (in a separate PR) IObservableRepositoriesClient (and probably IRepositoriesClient) would need to be changed also

Yep, there has been a move towards ensuring all property names for clients are singular (#53 #1035 #1036 #375 #1038),
so I agree these should be IOrganizationHooksClient Hook { get; private set; }

It also makes me realise we have quite a few other clients that still need to get resolved!

Opened #1207 to go into this a bit more

…leOrganizationHooksClient.cs

ryangribble · 2016-03-22T12:05:50Z

Octokit.Tests.Integration/fixtures/OrganizationsHooksFixture.cs

+    {
+        readonly IGitHubClient _github;
+        readonly OrganizationHook _hook;
+        private string organizationFixture;


local class variable should be named _organizationFixture and be readonly if possible

Hooks -> Hook fixtures-> Fixtures variable name changes in OrganizationsHooksFixture.cs

Norbo11 · 2017-07-31T09:46:15Z

Did the organization hooks client get removed at any point, or did it never make it into Octokit? It appears that GitHubClient.Organization.Hooks no longer exists...

ryangribble · 2017-08-01T20:15:11Z

Hi @Norbo11 this PR was not completed and thus hasn't been merged

AraHaan · 2019-05-06T03:08:22Z

So this will allow me to see if GitHub posts to a webhook I want to GitHub application to have to do specific stuff (like checking if the issue that posted to it has a specific label, is a pull request, edits specific files) and posts in it's status/checks?

Currently I got a untested and unrunnable ruby CI that I am not sure will work and I am moving it to .NET Core to C# that I know a lot more on.

shiftkey · 2019-09-16T13:13:58Z

Assigning myself to see if we can rebase this on top of master and address those conflicts

shiftkey · 2019-09-16T13:16:26Z

I also see #1884 has been opened and might be closer to merging

shiftkey · 2019-09-16T13:17:47Z

After a quick glance at #1884, I'm going to close this PR out in favour of the newer as it seems ready to review.

nubby109 added 3 commits March 9, 2016 17:34

Merge remote-tracking branch 'refs/remotes/octokit/master'

e27c2ac

Merge remote-tracking branch 'refs/remotes/octokit/master'

eb32a88

Added organization webhooks

481dcea

Added the organization webhooks and tests. Corrected a few repository url (related to hooks). Added a ensure condition for hookid in repository hooks

shiftkey added the feature label Mar 14, 2016

ryangribble added the feedback given label Mar 15, 2016

nubby109 added 3 commits March 20, 2016 19:42

resolving conflicts from octokit.net merge

7fa37e1

Merge remote-tracking branch 'refs/remotes/octokit/master' into org-w…

f372061

…ebhooks

1. Changed organizationName to org

7106944

2. Added EnsuresNonEmptyArguments() in OrganizationHooksClientTest.cs 3. Fixed the integrationTests I added for org webhooks 4. Changes asked by ryanribble

M-Zuber reviewed Mar 22, 2016
View reviewed changes

Removed extra parameter in description of delete function in Observab…

2cbc808

…leOrganizationHooksClient.cs

ryangribble removed the feedback given label Mar 22, 2016

ryangribble reviewed Mar 22, 2016
View reviewed changes

M-Zuber mentioned this pull request Mar 22, 2016

Lowercase namespace #1208

Closed

Making it more consistent

948c3cd

Hooks -> Hook fixtures-> Fixtures variable name changes in OrganizationsHooksFixture.cs

shiftkey assigned shiftkey and unassigned shiftkey Apr 4, 2016

MartinDawson mentioned this pull request Oct 7, 2018

organization web hooks #1884

Merged

shiftkey self-assigned this Sep 16, 2019

shiftkey removed their assignment Sep 16, 2019

shiftkey closed this Sep 16, 2019

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Org webhooks #1028 #1144

Org webhooks #1028 #1144

nubby109 commented Mar 12, 2016

nubby109 commented Mar 12, 2016

ryangribble commented Mar 13, 2016

nubby109 commented Mar 13, 2016

ryangribble commented Mar 13, 2016

shiftkey commented Mar 14, 2016

shiftkey commented Mar 14, 2016

nubby109 commented Mar 22, 2016

nubby109 commented Mar 22, 2016

M-Zuber Mar 22, 2016

nubby109 Mar 22, 2016

M-Zuber Mar 22, 2016

ryangribble Mar 22, 2016

M-Zuber Mar 22, 2016

nubby109 Mar 23, 2016

ryangribble Mar 22, 2016

nubby109 Mar 23, 2016

Norbo11 commented Jul 31, 2017

ryangribble commented Aug 1, 2017

AraHaan commented May 6, 2019

shiftkey commented Sep 16, 2019

shiftkey commented Sep 16, 2019

shiftkey commented Sep 16, 2019

Org webhooks #1028 #1144

Org webhooks #1028 #1144

Conversation

nubby109 commented Mar 12, 2016

nubby109 commented Mar 12, 2016

ryangribble commented Mar 13, 2016

nubby109 commented Mar 13, 2016

ryangribble commented Mar 13, 2016

shiftkey commented Mar 14, 2016

shiftkey commented Mar 14, 2016

nubby109 commented Mar 22, 2016

nubby109 commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Norbo11 commented Jul 31, 2017

ryangribble commented Aug 1, 2017

AraHaan commented May 6, 2019

shiftkey commented Sep 16, 2019

shiftkey commented Sep 16, 2019

shiftkey commented Sep 16, 2019