Add ability to specify GitHub events. #54

marcus-chan · 2015-04-27T22:27:47Z

Adds functionality to support other GitHub event types other than push. Extension of #53.

[FIXED JENKINS-23995] Set context for GH status

Fix issue with jobs using the CloudBees folders plugin being deleted

Some enhancements

... to deal with backward incompatible API change in github-api. The getUrl() method has changed in some of those methods to return API URL as opposed to HTML URL. This change fixes that.

KostyaSha · 2015-04-27T22:32:16Z

👎 coding style changes, can't follow real changes. Plugin is not about any other event types rather then PUSH.

jenkinsadmin · 2015-04-28T00:01:54Z

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

Fix formatting.

jtillman · 2015-04-28T19:36:25Z

👍 PUSH event seems real limited. In my setup, we just build during Tagging/Branch Creation. This makes sense for changes that go out in batch rather than individual items.

KostyaSha · 2015-04-28T19:47:17Z

This change has problems and useless complexity in every line of change, have no even enough time to comment them all.

@jtillman please open and describe your issue in jira.

jtillman · 2015-04-28T19:50:29Z

@KostyaSha I think my issues are already described in an existing ticket: https://issues.jenkins-ci.org/browse/JENKINS-27842

KostyaSha · 2015-04-28T19:56:14Z

@jtillman about "webhooks" vs "services" i already answered in your PR. If push ignores tags, then it other issue. Don't mix many issues in one.

This PR wants add extra configurations and end user complexity without describing any real case. Allowing register many events in plugin that doesn't support this events is bad behaviour. In the same way gh plugin can do coffee.

jtillman · 2015-04-28T20:25:34Z

Sorry but I couldn't fully understand your last explanation. What I think that the ticket references is the ability to subscribe different jobs to different repositories and events? Briefly looking at the UI change in this PR, it seems it just adds an advanced configuration which doesn't effect the existing use case. I'd be willing to help clean it up if its just stylistic issues. I'd like to understand better your concern around having this more configurable.

KostyaSha · 2015-04-28T20:43:08Z

Events registered by this plugin incoming into this plugin, registering useless event that doesn't handled by this plugin is redundant configuration without any real case. Listener is the right direction but UI configuration in this plugin absolutely useless.
Having variable for providing other variable optionally is also not good variant of UI configuration.
GitHubCause looks like a hack for his own single use case.
I split current plugin issues in 2 issues https://issues.jenkins-ci.org/browse/JENKINS-28138 and https://issues.jenkins-ci.org/browse/JENKINS-28139
And firstly i think should be resolved #39 , then migration to webhooks and finally extension.

KostyaSha · 2015-04-28T21:08:10Z

pom.xml

Jenkins has a bug https://issues.jenkins-ci.org/browse/JENKINS-18537 that has problems with JDK8.

Is it compile time issue of your java8 or runtime of all jenkins?

It's a runtime issue for Jenkins when using this plugin and running mvi hpi:run. The moment you startup Jenkins with this plugin, you should be able to see the issue.

Makes sense to decrease the dependency to 1.609.1 when it gets released

adepue · 2015-04-28T23:02:46Z

👍 Being able to wire to more than PUSH events would be nice for me as well.

dummy-lanwen-bot · 2016-07-24T08:24:06Z

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


-    // TODO: document me
+    @Deprecated
    public void onPost(String triggeredByUser);


"public" is redundant in this context.

dummy-lanwen-bot · 2016-07-24T08:24:14Z

SonarQube analysis reported 117 issues

2 blocker
1 critical
31 major
83 minor

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

Cleaner.java#L72: Catch Exception instead of Throwable.
GitHubPushTrigger.java#L194: Catch Exception instead of Throwable.
GitHubPushTrigger.java#L465: Make this "public static ALLOW_HOOKURL_OVERRIDE" field final
Cleaner.java#L64: Refactor the code to remove this label and the need for it.
Cleaner.java#L85: Move the "jenkins" string literal on the left side of this string comparison.
GitHubPushTrigger.java#L163: Add the missing @deprecated annotation.
GitHubPushTrigger.java#L185: Add the "@OverRide" annotation above this method signature
GitHubPushTrigger.java#L191: Refactor this code to not nest more than 3 if/for/while/switch/try statements.
GitHubPushTrigger.java#L409: Remove this use of constructor "String(byte[])"
GitHubPushTrigger.java#L465: Make ALLOW_HOOKURL_OVERRIDE a static final constant or non-public and provide accessors if needed.

KostyaSha and others added 17 commits February 22, 2015 05:14

[FIXED JENKINS-23995] Set commit status context

58a6148

Merge pull request jenkinsci#50 from KostyaSha/JENKINS-23995

598565c

[FIXED JENKINS-23995] Set context for GH status

[maven-release-plugin] prepare release github-1.11

00525dd

[maven-release-plugin] prepare for next development iteration

27a9d6f

Address JENKINS-25127 by using getAllIItems instead of getItems

2a49bac

Merge pull request jenkinsci#51 from ryangardner/master

4bb6115

Fix issue with jobs using the CloudBees folders plugin being deleted

[maven-release-plugin] prepare release github-1.11.1

5fd940c

[maven-release-plugin] prepare for next development iteration

6fae140

Simplify for loop readability

a0145d2

Move to separate method to have ability re-register hooks

c98e36f

[JENKINS-25127] Add Button for registering all hooks

6154ced

Merge pull request jenkinsci#52 from KostyaSha/enhance

575319b

Some enhancements

[maven-release-plugin] prepare release github-1.11.2

8e053ec

[maven-release-plugin] prepare for next development iteration

20fad8e

Bumping up to github-api 1.67

163acee

... to deal with backward incompatible API change in github-api. The getUrl() method has changed in some of those methods to return API URL as opposed to HTML URL. This change fixes that.

[maven-release-plugin] prepare release github-1.11.3

b2e2e6b

[maven-release-plugin] prepare for next development iteration

e5e5674

marcus-chan force-pushed the master branch from 7280bc3 to e122107 Compare April 28, 2015 18:36

Add ability to specify github events.

28f4766

Fix formatting.

marcus-chan force-pushed the master branch from e122107 to 28f4766 Compare April 28, 2015 18:47

KostyaSha reviewed Apr 28, 2015
View reviewed changes

dummy-lanwen-bot reviewed Jul 24, 2016
View reviewed changes

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

KostyaSha closed this Oct 7, 2019

jenkins-infra-bot mentioned this pull request Nov 12, 2017

[JENKINS-47938] Add Support for More Github Events #637

Open

Add ability to specify GitHub events. #54

Add ability to specify GitHub events. #54

Uh oh!

Conversation

marcus-chan commented Apr 27, 2015

Uh oh!

KostyaSha commented Apr 27, 2015

Uh oh!

jenkinsadmin commented Apr 28, 2015

Uh oh!

jtillman commented Apr 28, 2015

Uh oh!

KostyaSha commented Apr 28, 2015

Uh oh!

jtillman commented Apr 28, 2015

Uh oh!

KostyaSha commented Apr 28, 2015

Uh oh!

jtillman commented Apr 28, 2015

Uh oh!

KostyaSha commented Apr 28, 2015

Uh oh!

KostyaSha Apr 28, 2015

Choose a reason for hiding this comment

Uh oh!

marcus-chan Apr 28, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha Apr 28, 2015

Choose a reason for hiding this comment

Uh oh!

rogerhu Apr 29, 2015

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev May 16, 2015

Choose a reason for hiding this comment

Uh oh!

adepue commented Apr 28, 2015

Uh oh!

dummy-lanwen-bot Jul 24, 2016

Choose a reason for hiding this comment

Uh oh!

dummy-lanwen-bot commented Jul 24, 2016

Top 10 extra issues

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants