Skip GitHubOrgWebHook.register when using a GH app receiving all relevant events by jglick · Pull Request #289 · jenkinsci/github-branch-source-plugin

jglick · 2020-03-24T13:46:12Z

Tested that events sent to the app do work, but did not test the code in this PR.

…vant events

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java

sladyn98 · 2020-03-25T14:22:49Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubOrgWebHook.java

        return new File(Jenkins.get().getRootDir(), "github-webhooks/GitHubOrgHook." + orgName);
    }

+    // TODO never called?


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"

Yes but AFAICT this Java method is not called.

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 :/

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.

sladyn98 · 2020-03-25T14:32:14Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

+                    for (GHEventsSubscriber subscriber : GHEventsSubscriber.all()) {
+                        @SuppressWarnings("unchecked")
+                        Set<GHEvent> subscribedEvents = (Set<GHEvent>) eventsM.invoke(subscriber);
+                        if (!handledEvents.containsAll(subscribedEvents)) {


This would make sure no events go unhandled :+1

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.

I think this will break on PingGHEventSubscriber. Not yet tested.

docs/github-app.adoc

Co-Authored-By: Liam Newman <bitwiseman@gmail.com>

jglick · 2020-04-13T21:17:30Z

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.

docs/github-app.adoc

Co-Authored-By: Tim Jacomb <timjacomb1+github@gmail.com>

timja · 2020-04-13T21:19:11Z

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.

I haven't tried it

jglick · 2020-04-15T14:05:36Z

Note that besides this plugin, org.jenkinsci.plugins.github.webhook.WebhookManager also attempts to register webhooks and should ideally be made to shut up.

jglick · 2020-04-15T14:07:33Z

…which would probably require moving support for Apps into github-api, as suggested in #291 (comment).

jglick · 2020-04-24T21:38:29Z

also attempts to register webhooks and should ideally be made to shut up

jenkinsci/github-plugin#225

lifeofguenter · 2020-06-19T02:47:29Z

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

bitwiseman · 2020-12-23T21:52:13Z

@timja @jglick I
Is this still a valid PR? Would it be possible to clean it up and get it merged so we can include it in the release before the tables-to-divs change?

jglick · 2020-12-23T22:39:23Z

I believe it remains valid, just never found the time to test it. No time to touch it before January.

timja · 2020-12-24T07:59:21Z

@timja @jglick I
Is this still a valid PR? Would it be possible to clean it up and get it merged so we can include it in the release before the tables-to-divs change?

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)

bitwiseman · 2021-03-04T19:22:49Z

I have not updated this to deal with autoformatting.

The steps needed:

$ git merge task/formatting-base
$ mvnd spotless:apply && git commit -am "Apply autoformatting" && git merge -Xours upstream/master

jglick · 2021-03-04T19:35:45Z

$ git merge task/formatting-base
merge: task/formatting-base - not something we can merge

Huh? Do you just mean, say,

git pull origin master:master

resolve conflicts, reformat, and push?

bitwiseman · 2021-03-04T23:10:43Z

@jglick
I had pushed the task/formatter-base to my fork rather than this upstream.
I made a branch that is only the pom.xml change from master. I merge that and make sure it compiles. The the second line is safe to do and will contain only the formatting changes.

jglick · 2021-03-05T19:07:33Z

I made a branch that is only the pom.xml change from master. I merge that

Like

git checkout master -- pom.xml

if the PR did not already touch the POM?

bitwiseman · 2021-03-05T19:47:31Z

@jglick
Like that but also ensure that we've resolved all the potential non-formatting merge conflicts before making the formatting change.

…ource-plugin into app-webhooks

jglick · 2025-07-17T12:51:55Z

Also should check if https://github.com/jenkinsci/github-plugin/blob/0c86f968d0aa157c74fcd8b6477095ab1aab0883/src/main/java/com/cloudbees/jenkins/GitHubWebHook.java#L92-L115 https://github.com/jenkinsci/github-plugin/blob/0c86f968d0aa157c74fcd8b6477095ab1aab0883/src/main/java/com/cloudbees/jenkins/Cleaner.java#L21 can be suppressed.

Skip GitHubOrgWebHook.register when using a GH app receiving all rele…

49d22ad

…vant events

jglick requested review from sladyn98 and timja March 24, 2020 13:46

jglick mentioned this pull request Mar 24, 2020

Add support for GitHub app authentication #269

Merged

jglick commented Mar 24, 2020

View reviewed changes

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java Outdated Show resolved Hide resolved

sladyn98 reviewed Mar 25, 2020

View reviewed changes

bitwiseman reviewed Apr 13, 2020

View reviewed changes

docs/github-app.adoc Outdated Show resolved Hide resolved

Refining text about webhook config

283d4ab

Co-Authored-By: Liam Newman <bitwiseman@gmail.com>

timja reviewed Apr 13, 2020

View reviewed changes

docs/github-app.adoc Outdated Show resolved Hide resolved

Update docs/github-app.adoc

c43a8e4

Co-Authored-By: Tim Jacomb <timjacomb1+github@gmail.com>

bitwiseman added 3 commits March 4, 2021 16:02

Merge branch 'task/formatting-base' into app-webhooks

a749a1b

Apply autoformatting

b53e392

Merge remote-tracking branch 'upstream/master' into app-webhooks

4fe93ed

jglick mentioned this pull request Nov 10, 2021

feat(updating reregisterallhooks method): only register parent, not all children jenkinsci/github-plugin#241

Closed

jglick added 2 commits September 8, 2023 13:52

Merge commit '51545c3^' into app-webhooks

e00792b

Spotless like in jenkinsci#684

3cec758

jglick added 2 commits September 8, 2023 13:57

Merge commit '51545c3' into app-webhooks

f7513a6

Merge branch 'master' of https://github.com/jenkinsci/github-branch-s…

19599ce

…ource-plugin into app-webhooks

Conversation

jglick commented Mar 24, 2020

Uh oh!

Uh oh!

sladyn98 Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

jglick Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

sladyn98 Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

jglick Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

sladyn98 Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

jglick Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

jglick Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jglick commented Apr 13, 2020

Uh oh!

Uh oh!

timja commented Apr 13, 2020

Uh oh!

jglick commented Apr 15, 2020

Uh oh!

jglick commented Apr 15, 2020

Uh oh!

jglick commented Apr 24, 2020

Uh oh!

lifeofguenter commented Jun 19, 2020

Uh oh!

bitwiseman commented Dec 23, 2020

Uh oh!

jglick commented Dec 23, 2020

Uh oh!

timja commented Dec 24, 2020

Uh oh!

bitwiseman commented Mar 4, 2021

Uh oh!

jglick commented Mar 4, 2021

Uh oh!

bitwiseman commented Mar 4, 2021

Uh oh!

jglick commented Mar 5, 2021

Uh oh!

bitwiseman commented Mar 5, 2021

Uh oh!

jglick commented Jul 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants