[JENKINS-39590] Switch to `GitHub.parseEventPayload` for event parsing #155

stephenc · 2016-11-08T15:45:19Z

Work In Progress until hub4j/github-api#306 is merged and https://github.com/jenkinsci/github-api-plugin/ gets a release (also needs to be on top of #153)

This change is

oleg-nenashev

Maybe better to wait till the merge of the upstream PR before reviewing

oleg-nenashev · 2016-11-09T09:59:32Z

pom.xml

    <properties>
-        <jenkins.version>1.580</jenkins.version>
-        <jenkins-test-harness.version>1.580</jenkins-test-harness.version>
+        <jenkins.version>1.615</jenkins.version>


I would vouch for using LTS baseline like 1.625 since you go beyond the Java 6 compat threshold anyway

oleg-nenashev · 2016-11-09T10:01:45Z

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java

+                parseAssociatedNames((AbstractProject<?, ?>) item, result);
+            }
+        } else {
+            throw new AbstractMethodError("you must override the new overload of parseAssociatedNames");


see another PR

stephenc · 2016-11-09T13:26:27Z

@oleg-nenashev I created this PR (and tagged as WIP) so that the upstream PRs can be reviewed with an intended usage example... i.e. This PR

…parsing

stephenc · 2017-01-04T15:41:41Z

@reviewbybees

stephenc · 2017-01-09T21:24:36Z

@reviewbybees ping

KostyaSha · 2017-01-09T22:14:22Z

src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/PingGHEventSubscriber.java

     * @param payload payload of gh-event. Never blank
     */
    @Override
    protected void onEvent(GHEvent event, String payload) {


Does onEvent report errors itself? Then catch not needed.

github-plugin/src/main/java/org/jenkinsci/plugins/github/extension/GHEventsSubscriber.java

Lines 208 to 214 in 93d4069

try {

subscriber.onEvent(event, payload);

} catch (Throwable t) {

LOGGER.error("Subscriber {} failed to process {} hook, skipping...",

subscriber.getClass().getName(), event, t);

}

return null;

so try-catch useless wrapper, undo change and check that parser reports meaningful error.

That Throwable catch should be the last ditch catch. Propagation to it is not a good pattern.

I'll see if I can decipher what you mean tomorrow on computer not phone. But in general you should catch and log as close and as specific as possible while you know what sense to make...

In just you can trust to any underlying extension as it may throw NPE, so catching throwables and reporting is standard save ass pattern. If exception throws meaningful information then additional exception steps doesn't add anything useful for debugging.

KostyaSha

try-catch useless

lanwen · 2017-01-11T13:37:54Z

LGTM

ghost · 2017-01-11T18:38:14Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick · 2017-01-12T20:38:30Z

src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java

+                            String fullDisplayName = job.getFullDisplayName();
+                            LOGGER.debug("Considering to poke {}", fullDisplayName);
+                            if (GitHubRepositoryNameContributor.parseAssociatedNames(job)
+                                    .contains(changedRepository)) {


Please avoid gratuitous reformatting of otherwise unmodified lines.

stephenc · 2017-01-12T20:50:51Z

@reviewbybees done

lanwen · 2017-01-12T21:22:47Z

released as 1.25.1

oleg-nenashev reviewed Nov 9, 2016

View reviewed changes

KostyaSha force-pushed the master branch from be67249 to b82a666 Compare December 11, 2016 22:25

KostyaSha closed this Dec 11, 2016

KostyaSha reopened this Dec 11, 2016

[FIXED JENKINS-39590] Switch to GitHub.parseEventPayload for event …

0c5db21

…parsing

stephenc force-pushed the jenkins-39590 branch from d2299c3 to 0c5db21 Compare December 16, 2016 17:38

stephenc changed the title ~~[WIP] [JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing~~ [JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing Dec 16, 2016

KostyaSha reviewed Jan 9, 2017

View reviewed changes

KostyaSha requested changes Jan 9, 2017

View reviewed changes

[JENKINS-39590] Narrow the exception handling to payload parsing

a685508

jglick approved these changes Jan 12, 2017

View reviewed changes

lanwen merged commit 150fc93 into jenkinsci:master Jan 12, 2017

stephenc deleted the jenkins-39590 branch January 12, 2017 22:53

	try {
	subscriber.onEvent(event, payload);
	} catch (Throwable t) {
	LOGGER.error("Subscriber {} failed to process {} hook, skipping...",
	subscriber.getClass().getName(), event, t);
	}
	return null;

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing #155

[JENKINS-39590] Switch to GitHub.parseEventPayload for event parsing #155

Uh oh!

Conversation

stephenc commented Nov 8, 2016 • edited by KostyaSha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev Nov 9, 2016

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev Nov 9, 2016

Choose a reason for hiding this comment

Uh oh!

stephenc commented Nov 9, 2016

Uh oh!

stephenc commented Jan 4, 2017

Uh oh!

stephenc commented Jan 9, 2017

Uh oh!

KostyaSha Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

KostyaSha Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

stephenc Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

KostyaSha Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

KostyaSha left a comment

Choose a reason for hiding this comment

Uh oh!

lanwen commented Jan 11, 2017

Uh oh!

ghost commented Jan 11, 2017

Uh oh!

jglick Jan 12, 2017

Choose a reason for hiding this comment

Uh oh!

stephenc commented Jan 12, 2017

Uh oh!

lanwen commented Jan 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[JENKINS-39590] Switch to `GitHub.parseEventPayload` for event parsing #155

[JENKINS-39590] Switch to `GitHub.parseEventPayload` for event parsing #155

stephenc commented Nov 8, 2016 •

edited by KostyaSha

Loading