Skip GitHubOrgWebHook.register when using a GH app receiving all relevant events #289
Conversation
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
Outdated
Show resolved
Hide resolved
| @@ -95,6 +95,7 @@ private static File getTrackingFile(String orgName) { | |||
| return new File(Jenkins.get().getRootDir(), "github-webhooks/GitHubOrgHook." + orgName); | |||
| } | |||
|
|
|||
| // TODO never called? | |||
There was a problem hiding this comment.
The uninstallation of an app sends out an "action":"deleted" installation event, similar to installation of an app which also sends out an installation event but with "action":"created"
There was a problem hiding this comment.
Yes but AFAICT this Java method is not called.
There was a problem hiding this comment.
Yeah looking at it, it might be designed to act on a deregister for a webhook, but I cannot find any such event for it on the Github API document :/
There was a problem hiding this comment.
I think the intention was for this to be called when the OrganizationFolder was deleted, but from e424aa2 I am guessing this was never implemented.
| for (GHEventsSubscriber subscriber : GHEventsSubscriber.all()) { | ||
| @SuppressWarnings("unchecked") | ||
| Set<GHEvent> subscribedEvents = (Set<GHEvent>) eventsM.invoke(subscriber); | ||
| if (!handledEvents.containsAll(subscribedEvents)) { |
There was a problem hiding this comment.
This would make sure no events go unhandled :+1
There was a problem hiding this comment.
It might be too strict—to activate this patch you need to include the repository events which you might otherwise have skipped if you are only dealing with a small list of repositories—but I erred on the conservative side.
There was a problem hiding this comment.
I think this will break on PingGHEventSubscriber. Not yet tested.
Co-Authored-By: Liam Newman <[email protected]>
|
I have not tried this yet. Has anyone else? @bitwiseman if you would like to get this in, I can try to do a sanity check. |
Co-Authored-By: Tim Jacomb <[email protected]>
I haven't tried it |
|
Note that besides this plugin, |
|
…which would probably require moving support for Apps into |
|
|
This is related to #301 - e.g. currently a error message will popup not being able to create the webhook, even though the creation of that is not required |
|
I believe it remains valid, just never found the time to test it. No time to touch it before January. |
I think it’s still valid but probably not big enough to delay the tables to divs fix for it. ( the PR for retrieval via topics would be nice to look at though) |
|
I have not updated this to deal with autoformatting. The steps needed: |
$ git merge task/formatting-base
merge: task/formatting-base - not something we can mergeHuh? Do you just mean, say, git pull origin master:masterresolve conflicts, reformat, and push? |
|
@jglick |
Like git checkout master -- pom.xmlif the PR did not already touch the POM? |
|
@jglick |
See #269 (comment) for motivation.
Tested that events sent to the app do work, but did not test the code in this PR.