Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add push event information as arguments to triggered jobs. #169

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

matble
Copy link

@matble matble commented Apr 7, 2017

This change adds the git reference and head commit as arguments to triggered jobs. This allows Jenkins jobs to implement specialized logic. For example, a job can now have different behavior if the develop branch is pushed to vs. master.


This change is Reviewable

@lanwen
Copy link
Member

lanwen commented Apr 9, 2017

Hello! Thanks for your contribution, but this plugin only triggers polling, so you have already such information in your git. You can take a look to env parameters provided by git plugin.


// Schedule the job with the parameters and cause
QueueTaskFuture<?> build = asParameterizedJobMixIn(job)
.scheduleBuild2(0, new ParametersAction(values), new CauseAction(cause));
Copy link
Member

Choose a reason for hiding this comment

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

in any case, it should be EnvironmentContributingAction

Copy link
Author

Choose a reason for hiding this comment

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

Not entirely surely what you mean, do you mean add the parameters as environment variables only? Or add as environment variables as well as job parameters similar to what the Jenkins Github Pull Request Builder plugin does?

@matble
Copy link
Author

matble commented Apr 10, 2017

That is a good point. However, the polling logic is only going to give you things like latest commit and last branch pushed to, if two nearly simultaneous pushes occur then information is lost. It is also less intuitive to have to write that logic then simply have the head and reference passed directly into the job as environment variables or arguments. Each job trigger would come from exactly the correct place without the need for polling.

That does bring up the point that getting that information does supersede the need for polling. I didn't want to diverge from what the plugin is currently doing too much. Perhaps the injection of the parameters should be a separate option in the plugin entirely?

@lanwen
Copy link
Member

lanwen commented Apr 10, 2017

This trigger should be killed sometime :) It runs polling without any logic under the hood. So you can't change triggering behavior in an agile way.

Please look at new scm-api: https://github.com/jenkinsci/scm-api-plugin/blob/master/docs/implementation.adoc

GitHub should be a scm-source which provides events with complete info to something like an event bus. Then consumers subscribe to events, filter uninteresting and do the things you want.

I'm working on the implementation, but i spend only a few hours per week on this and it is in very beginning. So you can think about this kind of implementation too

@matble
Copy link
Author

matble commented Apr 10, 2017

I'm happy to take a look but if is a very large change I don't know how much time I would have to devote either. In either case, do you think this is a meaningful intermediate step?

At the very least, having the ref/head present as params/env variables after a poll allows the jobs to be more agile than they are now and removes the need for jobs have to logic to figure out last touched branch.

@lanwen
Copy link
Member

lanwen commented Apr 10, 2017

This change breaks binary compatibility and looks like a special case in general. So I don't think it could be merged in this form :(

To avoid branch computation logic in job, you can generate separate jobs with help of job-dsl plugin.

@matble
Copy link
Author

matble commented Apr 10, 2017

I'm happy to add a new configuration parameter to explicitly allow this to maintain backwards compatibility, although I don't know how breaking it really is. The plugin was triggering jobs without any arguments so the only possible argument values would be whatever was defined as "default" arguments.

There is really no elegant solution for this today. No solution to two pushes to different branches at same time, as well as having to use other plugins to get the same behavior. I think a lot of people run across this issue and have to work around it or accept as-is.

@KostyaSha
Copy link
Member

This change adds the git reference and head commit as arguments to triggered jobs. This allows Jenkins jobs to implement specialized logic. For example, a job can now have different behavior if the develop branch is pushed to vs. master.

Why not use normal trigger https://github.com/KostyaSha/github-integration-plugin/ ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants