[JENKINS-28138] Migration to webhooks with ext point #57

lanwen · 2015-06-16T19:11:23Z

Working to migrate to webhooks instead of services (JENKINS-28138)

The plan is

Incapsulate working with hooks in separate class
This can help to make process testable and simplify classes with other logic (Cleaner, Triggers)
Migrate to new webhook manager class in cleaner
Migrate to new manager in push trigger
Use in Cleaner queue interface instead of synchronized block. It can simplify add and fetch operations on how to add for cleaning new names
Clean old service hooks and replace it with webhooks on job reconfig or manual reregister btn
Add extension point to contribute new events to hook for some jobs
New events will be merged with old ones from existent hooks
~~[ ] Extend extension point to contribute hook events to make possible parse events it interested in~~ (will be done in separate PR)
Write tests

Next step is #60

KostyaSha · 2015-06-16T19:59:58Z

src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java

javadoc required

jenkinsadmin · 2015-06-17T00:01:23Z

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

…guava)

add javadocs add simple test for filtering by trigger

- default push event test - hook match predicate

…figuration

KostyaSha · 2015-07-03T12:28:30Z

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

useless without description

KostyaSha · 2015-07-03T15:20:57Z

src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java

this should be 1.12.0 then

better place TODO

KostyaSha · 2015-07-04T01:48:49Z

👍

KostyaSha · 2015-07-05T12:54:01Z

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

think nobody will be read this in generated javadoc format. Only here in code. So don't like <p/>

KostyaSha · 2015-07-05T12:59:09Z

src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java

Seems this class can be removed in future, mark with no external usage annotation?

KostyaSha · 2015-07-05T13:28:23Z

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

Skip run if queue not empty to allow end previous run?

think this logic should be investigated in separate pr

Just one check

if (!queue.isEmpty()) { return; }

so when first work finds non empty queue it just skip it. And no one will clean it ever :)

probably i mistaken. i thought that it handle only full run of cleaning (not single).

…t listeners

KostyaSha · 2015-07-05T14:20:12Z

src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java

oleg-nenashev · 2015-07-05T14:38:33Z

In general the code looks good to me (special kudos for extra tests). If the plugin works well (a manual testing is required), I would vote for such implementation

jglick · 2015-07-09T13:19:05Z

pom.xml

Are you actually changing dependencies in this diff hunk? If so, which? Please avoid gratuitous reformatting of files—keep the diff as short as possible. Makes your PR easier to review, and avoids unnecessary merge conflicts, cherry-pick conflicts, git-blame noise, etc.

Jessy, i'm "maintaining" last releases. I'm ok with this change. Also there is no any cherry-picks/merge conflicts/etc for this plugin.

git-blame and history will be more noisy when one small PR has 10 commits.

@jglick, I know about small diffs, thx! But this change alreay discussed with @KostyaSha

lanwen · 2015-07-11T22:43:31Z

@oleg-nenashev Manually tested over real GH

Log:

(when resave job with trigger)

июл 12, 2015 1:37:06 AM INFO org.jenkinsci.plugins.github.webhook.WebhookManager$1 run
GitHub webhooks activated for job test with [GitHubRepository[host=github.com,username=lanwen,repository=test]] (events: [PUSH])
июл 12, 2015 1:37:08 AM FINE org.jenkinsci.plugins.github.webhook.WebhookManager$2 apply
Hook already registered for events [PUSH]

or
(when remove trigger for job)

июл 12, 2015 1:41:05 AM FINE org.jenkinsci.plugins.github.webhook.WebhookManager unregisterFor
Check Repository:lanwen:test for redundant hooks...
июл 12, 2015 1:41:05 AM FINE org.jenkinsci.plugins.github.webhook.WebhookManager$3 apply
Deleted hook https://api.github.com/repos/lanwen/test/hooks/5281772 (events: [PUSH])

then
(when add new trigger)

GitHub webhooks activated for job test with [GitHubRepository[host=github.com,username=lanwen,repository=test]] (events: [PUSH])
июл 12, 2015 1:42:58 AM FINE org.jenkinsci.plugins.github.webhook.WebhookManager$3 apply
Created hook https://api.github.com/repos/lanwen/test/hooks/5281821 (events: [PUSH])
июл 12, 2015 1:42:59 AM FINE org.jenkinsci.plugins.github.webhook.GHEventHeader$PayloadHandler parse
Header X-GitHub-Event -> ping
июл 12, 2015 1:42:59 AM FINE org.jenkinsci.plugins.github.webhook.GHEventHeader$PayloadHandler parse
Unknown event - No enum constant org.kohsuke.github.GHEvent.PING
июл 12, 2015 1:42:59 AM FINEST org.jenkinsci.plugins.github.webhook.GHEventPayload$PayloadHandler parse
Payload {"zen":"Speak like a human.","hook_id":5281821,"hook":{"url":"https://api.github.com/repos/lanwen/test/hooks/5281821","test_url":"https://api.github.com/repos/lanwen/test/hooks/5281821/test","ping_url":"https://api.github.com/repos/lanwen/test/hooks/5281821/pings","id":5281821,"name":"web","active":true,"events":["push"],"config":{"url":"http://3352fb71.ngrok.com/jenkins/github-webhook/"},"last_response":{"code":null,"status":"unused","message":null},"updated_at":"2015-07-11T22:42:57Z","created_at":"2015-07-11T22:42:57Z"},"repository":{"id":38941520,"name":"test","full_name":"lanwen/test","owner":{"login":"lanwen","id":1964214,"avatar_url":"https://github.com/u/1964214?v=3","gravatar_id":"","url":"https://api.github.com/users/lanwen","html_url":"https://github.com/lanwen","followers_url":"https://api.github.com/users/lanwen/followers","following_url":"https://api.github.com/users/lanwen/following{/other_user}","gists_url":"https://api.github.com/users/lanwen/gists{/gist_id}","starred_url":"https://api.github.com/users/lanwen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lanwen/subscriptions","organizations_url":"https://api.github.com/users/lanwen/orgs","repos_url":"https://api.github.com/users/lanwen/repos","events_url":"https://api.github.com/users/lanwen/events{/privacy}","received_events_url":"https://api.github.com/users/lanwen/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/lanwen/test","description":"for test purposes","fork":false,"url":"https://api.github.com/repos/lanwen/test","forks_url":"https://api.github.com/repos/lanwen/test/forks","keys_url":"https://api.github.com/repos/lanwen/test/keys{/key_id}","collaborators_url":"https://api.github.com/repos/lanwen/test/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/lanwen/test/teams","hooks_url":"https://api.github.com/repos/lanwen/test/hooks","issue_events_url":"https://api.github.com/repos/lanwen/test/issues/events{/number}","events_url":"https://api.github.com/repos/lanwen/test/events","assignees_url":"https://api.github.com/repos/lanwen/test/assignees{/user}","branches_url":"https://api.github.com/repos/lanwen/test/branches{/branch}","tags_url":"https://api.github.com/repos/lanwen/test/tags","blobs_url":"https://api.github.com/repos/lanwen/test/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/lanwen/test/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/lanwen/test/git/refs{/sha}","trees_url":"https://api.github.com/repos/lanwen/test/git/trees{/sha}","statuses_url":"https://api.github.com/repos/lanwen/test/statuses/{sha}","languages_url":"https://api.github.com/repos/lanwen/test/languages","stargazers_url":"https://api.github.com/repos/lanwen/test/stargazers","contributors_url":"https://api.github.com/repos/lanwen/test/contributors","subscribers_url":"https://api.github.com/repos/lanwen/test/subscribers","subscription_url":"https://api.github.com/repos/lanwen/test/subscription","commits_url":"https://api.github.com/repos/lanwen/test/commits{/sha}","git_commits_url":"https://api.github.com/repos/lanwen/test/git/commits{/sha}","comments_url":"https://api.github.com/repos/lanwen/test/comments{/number}","issue_comment_url":"https://api.github.com/repos/lanwen/test/issues/comments{/number}","contents_url":"https://api.github.com/repos/lanwen/test/contents/{+path}","compare_url":"https://api.github.com/repos/lanwen/test/compare/{base}...{head}","merges_url":"https://api.github.com/repos/lanwen/test/merges","archive_url":"https://api.github.com/repos/lanwen/test/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/lanwen/test/downloads","issues_url":"https://api.github.com/repos/lanwen/test/issues{/number}","pulls_url":"https://api.github.com/repos/lanwen/test/pulls{/number}","milestones_url":"https://api.github.com/repos/lanwen/test/milestones{/number}","notifications_url":"https://api.github.com/repos/lanwen/test/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/lanwen/test/labels{/name}","releases_url":"https://api.github.com/repos/lanwen/test/releases{/id}","created_at":"2015-07-11T21:47:22Z","updated_at":"2015-07-11T21:47:22Z","pushed_at":"2015-07-11T22:21:07Z","git_url":"git://github.com/lanwen/test.git","ssh_url":"[email protected]:lanwen/test.git","clone_url":"https://github.com/lanwen/test.git","svn_url":"https://github.com/lanwen/test","homepage":null,"size":0,"stargazers_count":0,"watchers_count":0,"language":null,"has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"},"sender":{"login":"lanwen","id":1964214,"avatar_url":"https://github.com/u/1964214?v=3","gravatar_id":"","url":"https://api.github.com/users/lanwen","html_url":"https://github.com/lanwen","followers_url":"https://api.github.com/users/lanwen/followers","following_url":"https://api.github.com/users/lanwen/following{/other_user}","gists_url":"https://api.github.com/users/lanwen/gists{/gist_id}","starred_url":"https://api.github.com/users/lanwen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lanwen/subscriptions","organizations_url":"https://api.github.com/users/lanwen/orgs","repos_url":"https://api.github.com/users/lanwen/repos","events_url":"https://api.github.com/users/lanwen/events{/privacy}","received_events_url":"https://api.github.com/users/lanwen/received_events","type":"User","site_admin":false}}
июл 12, 2015 1:42:59 AM INFO org.jenkinsci.plugins.github.webhook.RequirePostWithGHHookPayload$Processor logPingEvent
Got ping event from GH

amuniz · 2015-07-15T12:12:38Z

Does this PR require #60 to be merged or vice versa?

lanwen · 2015-07-15T13:01:35Z

@amuniz yes, this feature was splitted for two prs to simplify review.

KostyaSha · 2015-07-15T13:51:09Z

src/main/java/org/jenkinsci/plugins/github/util/JobInfoHelpers.java

rename to isApplicable()?

this means that job alive in terms of github hook. So "applicable" word is not clarify meaning

KostyaSha reviewed Jun 16, 2015
View reviewed changes

src/main/java/org/jenkinsci/plugins/github/webhook/WebhookManager.java Outdated

Copy link

Member

KostyaSha Jun 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javadoc required

MerkushevKirill added 3 commits June 17, 2015 03:28

copypaste from guava's fluent iterable class (for not to update full …

45a2229

…guava)

new classes to work with hooks - manager, ext point and job utility

a38c6ef

refactored cleaner and push trigger to work with hook manager

f02238e

lanwen force-pushed the webhooks branch from a0c6ab7 to f02238e Compare June 17, 2015 00:33

since jdoc + fix logger class

cba5353

lanwen changed the title ~~[Not for merge] Migration to webhooks with ext point to add intereste…~~ [JENKINS-28138][Not for merge now] Migration to webhooks with ext point Jun 17, 2015

KostyaSha changed the title ~~[JENKINS-28138][Not for merge now] Migration to webhooks with ext point~~ WIP [JENKINS-28138] Migration to webhooks with ext point Jun 23, 2015

MerkushevKirill added 6 commits July 3, 2015 03:07

move to separate class default gh-push-event listener

03b0917

add javadocs add simple test for filtering by trigger

add tests for whook manager

eedfad7

add dependency to hamcrest for test purposes

6a08dee

tests for webhook manager

c60db37

- default push event test - hook match predicate

get rid of event parser in listener for now

8fed502

process ping event in simple way

09add52

lanwen changed the title ~~WIP [JENKINS-28138] Migration to webhooks with ext point~~ [JENKINS-28138] Migration to webhooks with ext point Jul 3, 2015

throw and catch config exception on mailformed hook url in global con…

b926283

…figuration

lanwen force-pushed the webhooks branch from e6df3bd to b926283 Compare July 3, 2015 12:21

KostyaSha reviewed Jul 3, 2015
View reviewed changes

src/main/java/com/cloudbees/jenkins/Cleaner.java Outdated

Copy link

Member

KostyaSha Jul 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useless without description

lanwen force-pushed the webhooks branch from c676905 to 307e841 Compare July 3, 2015 12:31

KostyaSha mentioned this pull request Jul 3, 2015

[JENKINS-27136] Workflow support for GitHub Push Triggers #59

Closed

KostyaSha reviewed Jul 3, 2015
View reviewed changes

lanwen force-pushed the webhooks branch from 307e841 to 0926211 Compare July 3, 2015 15:31

cleanup useless jdocs

0c35c25

lanwen force-pushed the webhooks branch from 0926211 to 0c35c25 Compare July 5, 2015 12:52

KostyaSha reviewed Jul 5, 2015
View reviewed changes

lanwen force-pushed the webhooks branch from 428c0f0 to f916a82 Compare July 5, 2015 13:27

KostyaSha reviewed Jul 5, 2015
View reviewed changes

rename alive - aliveRepos in cleaner and WHManager

0924fae

lanwen force-pushed the webhooks branch from f916a82 to 0924fae Compare July 5, 2015 13:45

make configurable alive state of job for cleaner with help of gh even…

c04604b

…t listeners

KostyaSha reviewed Jul 5, 2015
View reviewed changes

src/main/java/org/jenkinsci/plugins/github/util/FluentIterableWrapper.java Outdated

Copy link

Member

KostyaSha Jul 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

log in debug ping event

ba83b1b

rename GHEventsListener to Subscriber and move ext point to separate pkg

83abac5

lanwen force-pushed the webhooks branch from 37de708 to 83abac5 Compare July 5, 2015 16:43

lanwen mentioned this pull request Jul 5, 2015

[JENKINS-28139] Hook payload parse extension point #60

Merged

jglick reviewed Jul 9, 2015
View reviewed changes

KostyaSha reviewed Jul 15, 2015
View reviewed changes

oleg-nenashev mentioned this pull request Jul 15, 2015

Add ability to specify GitHub events. #54

Closed

KostyaSha merged commit 83abac5 into jenkinsci:master Jul 15, 2015

[JENKINS-28138] Migration to webhooks with ext point #57

[JENKINS-28138] Migration to webhooks with ext point #57

Uh oh!

Conversation

lanwen commented Jun 16, 2015

Next step is #60

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenkinsadmin commented Jun 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Jul 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Jul 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanwen commented Jul 11, 2015

Uh oh!

amuniz commented Jul 15, 2015

Uh oh!

lanwen commented Jul 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants