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

amuniz · 2015-07-03T14:30:48Z

https://issues.jenkins-ci.org/browse/JENKINS-27136

Currently Workflow jobs can not be triggered when a GitHub push hook is received, this PR makes it possible.

In this document there is information about the required changes in plugins to make them compatible with Workflow. This changes have been applied here.

@reviewbybees

KostyaSha · 2015-07-03T14:33:13Z

pom.xml

The minimun Workflow compatible core version is 1.580.1

compatible with what?

You need to be running a sufficiently recent Jenkins release: an LTS in the 1.580.x line or newer

Any Workflow related feature requires core 1.580.X. Since I'm adding Workflow compatibility, the plugin requires 1.580.X now.

See https://github.com/jenkinsci/workflow-plugin/blob/master/README.md#installation

@KostyaSha

This doesn't mean that you must update core dependencies in non workflow plugins

SCMTriggerItem was added in 1.568. 1.580.x is the next LTS after that.

And this is right answer

@amuniz a tip: if you change this line to

<version>1.580.1</version> 

then this whole comment thread becomes hidden by default. :-)

@jglick 🙀

ghost · 2015-07-03T14:34:05Z

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.

s0undt3ch · 2015-07-03T14:38:49Z

I'm realy after this ... 👍

KostyaSha · 2015-07-03T14:40:52Z

👎 until #57 will be merged. Also this change looks like breaking interfaces change.

KostyaSha · 2015-07-03T14:41:34Z

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

amuniz · 2015-07-03T14:48:14Z

@KostyaSha the only way to make this compatible with Workflow jobs is changing interfaces. Not sure how this impact in #57

lanwen · 2015-07-03T15:00:40Z

please wait for #57 because of hard rebase process instead

tfennelly · 2015-07-03T15:30:15Z

@amuniz for the methods that were modified, maybe you could override them instead, deprecating the old method signatures?

tfennelly · 2015-07-03T15:32:25Z

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

I think you'll need to add a null check on Jenkins.getInstance(). Otherwise, we'll be getting FindBugs errors I think.

new jenkins has getJenkinsInstance instead

Starting from 1.596 only AFAIK

Then just copy-paste it realisation and put to static function imho.

jenkinsadmin · 2015-07-03T18:01:12Z

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

KostyaSha · 2015-07-04T01:23:03Z

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

please don't collapse in one line, really difficult read.

…h JenkinsRule load

…on is required) + fix test

…dencies

jglick · 2015-07-08T20:05:53Z

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

This will be a CCE if it is not in fact a ParameterizedJob. 🐛

jglick · 2015-08-11T17:16:38Z

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

Another place where the 1.621 method would be nicer.

jglick · 2015-08-11T17:18:44Z

🐝

jglick · 2015-08-11T17:55:58Z

🐝

oleg-nenashev · 2015-08-11T20:22:36Z

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

Seems we need the withBridgeMethods magic here as well

oleg-nenashev · 2015-08-11T20:25:32Z

@amuniz
Did you agree on the binary compatibility impact with @lanwen and @KostyaSha . There're some issues in the PR, but IIRC there was a formal breakage in the previous release. Probably the compatibility loss is OK now.

lanwen · 2015-08-11T21:57:57Z

i'm ok to merge it. But @KostyaSha thinks that bumping core version before all other useful PRs will be closed on current core version may cause this plugin unusable for older jenkins instances.

(it should be squashed to limited set of commits before merge happens, as of 30 commits for such small change is strange, but please wait until green light)

jglick · 2015-08-11T22:35:38Z

If the plugin maintainer desires commit squashing I would suggest that be done as part of the merge rather than destroying history in the PR itself.

KostyaSha · 2015-08-12T09:49:15Z

@jglick i can suggest you firstly work with recena and amuniz PRs in CB forks and only after review send to maintainers. Never saw in jenkinsci PRs with so many commits and issues before. Back to upstream repos when their PRs wouldn't have "child" issues.
I'm always squashing my commits and i have no time for doing it instead of your full time employees.

@oleg-nenashev could you say what exactly was broken for you in last releases?

amuniz · 2015-08-12T10:10:24Z

@KostyaSha just ask for squash before you are going to merge, I'll do it.

The current amount of revisions and comments in every PR is hopefully going to significantly increase the code base quality in the Jenkins community in general. Moreover, I think is quite good for other developers to have all this review information available publicly, since it is, from my point if view, a nice source of knowledge.

KostyaSha · 2015-08-12T10:22:45Z

@amuniz CB pays you for your work, but it doesn't mean that CB that hired and locked all possible jenkins related devs must lock jenkins activities on their internal plans/needs. This noise is waste of time for FOSS devs, instead of working on new features and other plugins i'm getting 100 comments with newbie errors. Compare to @jglick PRs that doesn't require many discussions.

I will back to this PR when we resolve other planned features/fixes and will be ok with core bump.

daniel-beck · 2015-08-15T19:44:49Z

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

This change makes the Javadoc comment in line 133 obsolete, and without that, AbstractProject is an unused import as well.

@daniel-beck thanks. Fixed.

Conflicts: src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java

amuniz · 2015-08-17T12:21:54Z

Merge conflicts resolved.

lanwen · 2015-08-17T14:35:56Z

@amuniz please don't rebase. We have 3 or 4 prs in "must have" queue before we are going to merge this pr. I'll do the rebase by myself when the time will come

amuniz · 2015-08-24T18:38:47Z

@lanwen ok. If you face some issue while merging, ping me.

amuniz · 2015-09-01T11:01:16Z

@lanwen is there any pending PR blocking this to merge yet?

KostyaSha · 2015-09-01T11:12:41Z

@amuniz core update blocks it.

amuniz · 2015-09-01T12:05:48Z

@KostyaSha could you elaborate please? What's wrong with the core update?

KostyaSha · 2015-09-01T12:29:31Z

1.554.1 (2014/04/30)
1.565.3 (2014/10/01)
1.580.1 (2014/10/29)

So soon it will be possible to bump new version
Not all planned features are done, if we will change something after, you will jump with "backward compatibility is broken" so better wait. I don't remember exact plan, but notifyCommit and generic push trigger is one of.

lanwen · 2015-09-01T13:05:54Z

no, think generic trigger is too complex to move workflow in queue.

And while git-plugin of new version is already depends on core version with workflow think we should try to merge this.

KostyaSha · 2015-09-01T13:06:59Z

ok, as you wish, i tired from this workflow spam

lanwen · 2015-09-03T15:42:47Z

please review #82

lanwen · 2015-09-03T22:02:19Z

Merged in #82

Antonio Muñiz added 3 commits July 1, 2015 22:46

[JENKINS-27136] Initial interfacing adapt

8b6eb63

[JENKINS-27136] More interfacing and compatibility changes

0042145

[JENKINS-27136] Keep MultiSCM compatibility + remove unused imports

6f158b0

KostyaSha reviewed Jul 3, 2015
View reviewed changes

src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.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.

formating

Fix formatting

9897530

tfennelly reviewed Jul 3, 2015
View reviewed changes

KostyaSha reviewed Jul 4, 2015
View reviewed changes

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

Copy link

Member

KostyaSha Jul 4, 2015

Choose a reason for hiding this comment

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

please don't collapse in one line, really difficult read.

Antonio Muñiz and others added 6 commits July 6, 2015 12:19

Fixing formatting issues and findbugs

a9fe2e7

[JENKINS-27136] Adding tests

c300380

[JENKINS-27136] Removing not used dependency that was interfering wit…

bbf0247

…h JenkinsRule load

[JENKINS-27136] Remove jgit dependency (git plugin decides what versi…

57a5b4e

…on is required) + fix test

[JENKINS-27136] Receive hook triggering WF job test + fix wrong depen…

f014b31

…dencies

[JENKINS-27136] Fixing Workflow test

eaa1de6

jglick reviewed Jul 8, 2015
View reviewed changes

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

Copy link

Member

jglick Jul 8, 2015

Choose a reason for hiding this comment

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

This will be a CCE if it is not in fact a ParameterizedJob. 🐛

jglick reviewed Aug 11, 2015
View reviewed changes

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

Copy link

Member

jglick Aug 11, 2015

Choose a reason for hiding this comment

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

Another place where the 1.621 method would be nicer.

amuniz added 2 commits August 11, 2015 19:23

Add TODOs to keep in mind when upgrading to a new core version

d197cb0

More TODOs

c7b2ad6

oleg-nenashev reviewed Aug 11, 2015
View reviewed changes

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

Copy link

Member

oleg-nenashev Aug 11, 2015

Choose a reason for hiding this comment

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

Seems we need the withBridgeMethods magic here as well

daniel-beck reviewed Aug 15, 2015
View reviewed changes

amuniz added 2 commits August 16, 2015 18:58

Fix obsolete javadoc

6fbb9a5

Merge branch 'master' into gh-trigger-JENKINS-27136

612072a

Conflicts: src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java

lanwen mentioned this pull request Sep 3, 2015

[FIXES JENKINS-27136] Workflow plugin support #82

Merged

lanwen closed this Sep 3, 2015

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

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

Uh oh!

Conversation

amuniz commented Jul 3, 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!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 3, 2015

Uh oh!

s0undt3ch commented Jul 3, 2015

Uh oh!

KostyaSha commented Jul 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amuniz commented Jul 3, 2015

Uh oh!

lanwen commented Jul 3, 2015

Uh oh!

tfennelly commented Jul 3, 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!

jenkinsadmin commented Jul 3, 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!

jglick commented Aug 11, 2015

Uh oh!

jglick commented Aug 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleg-nenashev commented Aug 11, 2015

Uh oh!

lanwen commented Aug 11, 2015

Uh oh!

jglick commented Aug 11, 2015

Uh oh!

KostyaSha commented Aug 12, 2015

Uh oh!

amuniz commented Aug 12, 2015

Uh oh!

KostyaSha commented Aug 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amuniz commented Aug 17, 2015

Uh oh!