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

[JENKINS-26010] Workflow compatibility #240

Merged
merged 51 commits into from Aug 15, 2015

Conversation

Projects
None yet
10 participants
@tfennelly
Copy link
Member

commented Jul 10, 2015

See JENKINS-26010

Current status: Ready for review. No more changes planned.

  • AbstractProject to Job in core code
  • Upgrade rebuild-plugin
  • Upgrade git-plugin
  • AbstractBuild to Run in core code
  • Fix tests
  • Manual tests
  • Resolve TODOs
  • Build status "Verified" event back to Gerrit Server.
  • Backward Compatibility .
  • Run acceptance tests
  • Create JIRA for adding acceptance tests (Freestyle and Workflow jobs). See JENKINS-29836.
  • Create JIRA for adding code checkout feature i.e. an easy way to trigger checkout of the correct branch/revision of code that triggered the build. see JENKINS-29838.
  • Document how to checkout the correct branch/revision of code that triggered the build. See wiki docs.
  • Create test plugin to test backward compatibility.
@@ -118,7 +118,7 @@ public static DependencyQueueTaskDispatcher getInstance() {
@Override
public CauseOfBlockage canRun(Queue.Item item) {
//AbstractProject check
if (!(item.task instanceof AbstractProject)) {
if (!(item.task instanceof Job)) {
logger.debug("Not an abstract project: {}", item.task);

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

The log is a lie :)

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 10, 2015

Author Member

Thanks.

@@ -49,7 +49,7 @@
* @param build Triggered build to provide custom message for
* @return the custom message
*/
public String getBuildStartedMessage(AbstractBuild build) {
public String getBuildStartedMessage(Run build) {

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 10, 2015

Author Member

I hadn't seen that one, but I know there are some issues lying ahead for some other external sony lib that assumes AbstractBuild or AbstractProject. I forget the name of it right now but will tell you when I hit it again.

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 10, 2015

Author Member

I hope we can fix those and cut new releases of them, yeah?

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

Yes I'm a maintainer of gerrit-events as well :)

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

But that lib should have no dependency on Jenkins at all.

FilePath path = matches[0];
content = this.getExpandedContent(path, envVars);
logger.info("Obtained failure message from file: {}", content);
// TODO: what will we do here?

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

I've seen code something like getCurrentExecutor(build).getWorkspace() appear now and then.

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 10, 2015

Member

Or build.getCurrentExecutor().getWorkspace() but getCurrentExecutor() will always return null unless there were a build in progress.

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2015

Member

There is no getCurrentExecutor anywhere in Jenkins core, so I am not sure what you could mean by that.

Currently there is nothing else to be done here except check for an instance of AbstractBuild. JENKINS-26138 might involve introducing a core API whereby you could enumerate workspaces associated with a Run.

In the meantime, it seems reasonable to just fall back to a message that does not rely on knowledge of workspace files.

projectbuildDelay,
cause,
badgeAction,
new RetriggerAction(cause.getContext()),
new RetriggerAllAction(cause.getContext()),
parameters);
} else if (project instanceof SCMTriggerItem) {
// TODO: Using SCMTriggerItem smells bad here. What should we be using?

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 10, 2015

Member

Could something like this be used here?

ParameterizedJobMixIn scheduler = new ParameterizedJobMixIn() {
    @Override protected Job asJob() {
        return project;
    }
};
scheduler.scheduleBuild(projectbuildDelay)

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 14, 2015

Contributor

This causes a regression for Workflows because we don't actually include the parameters.

We can pass all the actions, and at minimum we should do that even if we stick to SCMTriggerItem. You failed to pass badgeAction, and both Retrigger actions, as well as the parameters.

We can't pass the cause directly though, at least not to SCMTriggerItem

This should also fix the lack of re-trigger options for the workflow as well.

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2015

Member

You need to use ParameterizedJobMixIn.scheduleBuild2. This handles all (schedulable!) project types. Then just add a TODO comment reminding the plugin maintainer to use the simplified static convenience method as of 1.621.

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2015

Member

We can't pass the cause directly though

Just use new CauseAction(cause).

@rsandell

This comment has been minimized.

Copy link
Member

commented Jul 10, 2015

@scoheb, @GLundh, @TWestling I'm going on vacation today, so I won't be much help to @tfennelly during that time. Could you help him out with some friendly review?

@GLundh

This comment has been minimized.

Copy link
Member

commented Jul 10, 2015

@rsandell: I would love to help out, but I'm going on vacation myself today, and wont be able to spend much time on it until early august.

@scoheb

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2015

@rsandell ... I can definitely lend a hand!

@jacob-keller

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2015

@tfennelly I'm willing to attempt running a version as well, to see how it goes. Let me know what I can do to help. Thanks for taking a serious stab at this one.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2015

@scoheb @GLundh @jacob-keller Thanks guys 🍺 ... I'll ping ye as soon as I have something testable.

} else if (project.getHasCustomQuietPeriod()
&& project.getQuietPeriod() > projectbuildDelay) {
projectbuildDelay = project.getQuietPeriod();
} else if (project instanceof AbstractProject) {

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

ParameterizedJobMixing.ParameterizedJob which both AbstractProject and Workflow implements has quietPeriod methods IIRC.

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 10, 2015

Author Member

Rights, but it doesn't have AbstractProject.getHasCustomQuietPeriod. We'd have to drop that. Would that be a terrible thing and cause regressions?

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 10, 2015

Author Member

Note getQuietPeriod defaults if not set.

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 10, 2015

Contributor

I personally don't think that's an issue.
On Jul 10, 2015 9:42 AM, "Tom Fennelly" notifications@github.com wrote:

In
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/EventListener.java
#240 (comment)
:

     BadgeAction badgeAction = new BadgeAction(event);
     //during low traffic we still don't want to spam Gerrit, 3 is a nice number, isn't it?
     int projectbuildDelay = t.getBuildScheduleDelay();
     if (cause instanceof GerritUserCause) {
         // it's a manual trigger, no need for a quiet period
         projectbuildDelay = 0;
  •    } else if (project.getHasCustomQuietPeriod()
    
  •            && project.getQuietPeriod() > projectbuildDelay) {
    
  •        projectbuildDelay = project.getQuietPeriod();
    
  •    } else if (project instanceof AbstractProject) {
    

Note getQuietPeriod defaults if not set.


Reply to this email directly or view it on GitHub
https://github.com/jenkinsci/gerrit-trigger-plugin/pull/240/files#r34372420
.

This comment has been minimized.

Copy link
@rinrinne

rinrinne Jul 10, 2015

Member

If not check AbstractProject.getHasCustomQuietPeriod, ParameterizedJob.getQuietPeriod might return system value given by Jenkins.getQuietPeriod if quietPeriod in project is not set. system's default is 5.

BTW, can we use GT's own period only?

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 10, 2015

Contributor

So, if I understand this right, the old behavior was something like:

  1. If we're manual trigger, don't delay at all
  2. If the job has a custom quiet period (ie: not default) AND that period is larger than the ScheduleDelay, use this
  3. Otherwise, use the ScheduleDelay

So what's wrong with using the quietPeriodDelay even if it's not customized? Why can't we also check if we're above the default?

Is it so that GT's custom delay overrides the global default, but not per-jobs?

I'd rather use the longest of all configured delays, not the shortest. Ie: GT's delay only overrides global if the global is shorter?

We could also use only GT's trigger period, without worrying about per-job settings?

Thoughts?

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 13, 2015

Author Member

@jacob-keller @rinrinne guys, I'm afraid I don't really have a valuable opinion on stuff like this. I will be relying on ye to tell me what makes most sense from a Gerrit pov.

return (GerritTrigger)project.getTrigger(GerritTrigger.class);

// TODO: I'd imagine this needs to be optimized?
DescribableList<Trigger<?>, TriggerDescriptor> triggers =

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

ParameterizedJobMixing.ParameterizedJob has a getTriggers() method.

@@ -1918,8 +1928,12 @@ public synchronized void scheduled(ChangeBasedEvent event, ParametersAction para
* The event that originally triggered the build.
*/
private void cancelJob(GerritTriggeredEvent event) {
if (!(job instanceof Queue.Task)) {
logger.error("Error canceling job. The job is not of type Task.");
}

This comment has been minimized.

Copy link
@rsandell

rsandell Jul 10, 2015

Member

I guess you should return directly after the logging?

And please log the task as well, so the admin knows which of the 10k jobs the log is talking about :)

@rsandell

This comment has been minimized.

Copy link
Member

commented Jul 10, 2015

@scoheb @jacob-keller Thanks guys! Then @tfennelly is in good hands while I relax with some 🍻 myself :)


// TODO: find a way to handle add/remove of triggers
if (!(job instanceof AbstractProject)) {
return;

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 10, 2015

Member

What would happen if it's an instance of WorkflowJob?

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 10, 2015

Contributor

Does WorkflowJob have a way to add and remove triggers? I assume we'd want something similar to that? I know that the ParameterizedJobMixin does not have support for add triggers, but maybe we can just instanceOf to WorkflowJob here?

According to the source for WorkflowJob.java, it appears that WorkflowJob has an "addTrigger" function and getTriggers function, but this isn't exposed by the Mixin API. It would feel really awkward to instanceof and then use the same code both times.... Also, it doesn't support "getTrigger" standalone only the getTriggers...

Kind of ugly either way.

This comment has been minimized.

Copy link
@amuniz

amuniz Jul 13, 2015

Member

The getTriggers() method is defined in ParameterizedJobMixIn.ParameterizedJob. We can cast to this one better than WorkflowJob.

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 13, 2015

Author Member

@amuniz At the moment, nothing will happen for a Workflow Job i.e. add/remove of triggers is not supported for workflow as things stand. That's why I marked it with a TODO. We need to find a way of handling it for workflow. AbstractProject has methods for adding and removing triggers, but that's no help for a Workflow job.

Trigger rename might be fixable by finding the current trigger and renaming the server on it Vs removing and readding the trigger (at the moment, rename is supported by removing the trigger and adding a new one). Not sure yet how remove will be supported (when a Gerrit server is removed we need to find all Jobs with triggers for that server and remove them).

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 13, 2015

Contributor

you can add a trigger easily. It seems like the trigger add code will remove the old trigger already. Sadly WorkflowJob.java does not export a remove trigger function like the code does. Maybe we just don't bother removing the old trigger at all when a server is removed?

What's the reason that we actually have remove/add trigger code here specifically? I presume to prevent a bug? What do other trigger plugins do to prevent this sort of issue?

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 27, 2015

Author Member

Auto removing zombie triggers after a Gerrit server is removed seems to make sense to me.

In any case (and imo), the debate on the pros and cons of programmatically removing a trigger is something that should happen separate to this PR. IMO, our job here is to integrate workflow and maintain the same feature set. It shouldn't be about proposing fundamental plugin design changes.

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 27, 2015

Contributor

On Jul 27, 2015 12:48 AM, "Tom Fennelly" notifications@github.com wrote:

In
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritServer.java:

@@ -914,14 +915,21 @@ private void rename(String newName) {
* @param oldName the old name of the Gerrit server
*/
private void changeSelectedServerInJobs(String oldName) {

  •    for (AbstractProject job :
    
    PluginImpl.getConfiguredJobs_(oldName)) {
  •        GerritTrigger trigger =
    
    (GerritTrigger)job.getTrigger(GerritTrigger.class);
  •    for (Job job : PluginImpl.getConfiguredJobs_(oldName)) {
    
  •        // TODO: find a way to handle add/remove of triggers
    
  •        if (!(job instanceof AbstractProject)) {
    
  •            return;
    

Auto removing zombie triggers after a Gerrit server is removed seems to
make sense to me.

In any case (and imo), the debate on the pros and cons of
programmatically removing a trigger is something that should happen
separate to this PR. IMO, our job here is to integrate workflow and
maintain the same feature set. It shouldn't be about proposing fundamental
plugin design changes.


Reply to this email directly or view it on GitHub.

While that is true.. I don't know of a good way to implement this
functionality and it may make sense to do the PR to remove such a feature
first unless we can actually do this change..

One way might be to optionally depend on work flow plug in. I have seen
others do this but I don't know how they did it.

This comment has been minimized.

Copy link
@tfennelly

tfennelly Jul 27, 2015

Author Member

@jacob-keller Right, I didn't see a way of doing it for workflow. So, the easiest thing (for now) might be to mark it as a missing feature for Workflow jobs and not have that point block this PR?

Then, separately have a discussion on the merits/demerits etc of auto-removing triggers. This will either result in a task to remove the feature completely (for all build types), or a task to find a way of making it work for workflow.

This comment has been minimized.

Copy link
@jacob-keller

jacob-keller Jul 27, 2015

Contributor

I think that is fine myself. I would rather not block trigger support just
because we are missing a relatively minor feature.
On Jul 27, 2015 3:42 AM, "Tom Fennelly" notifications@github.com wrote:

In
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritServer.java
#240 (comment)
:

@@ -914,14 +915,21 @@ private void rename(String newName) {
* @param oldName the old name of the Gerrit server
*/
private void changeSelectedServerInJobs(String oldName) {

  •    for (AbstractProject job : PluginImpl.getConfiguredJobs_(oldName)) {
    
  •        GerritTrigger trigger = (GerritTrigger)job.getTrigger(GerritTrigger.class);
    
  •    for (Job job : PluginImpl.getConfiguredJobs_(oldName)) {
    
  •        // TODO: find a way to handle add/remove of triggers
    
  •        if (!(job instanceof AbstractProject)) {
    
  •            return;
    

@jacob-keller https://github.com/jacob-keller Right, I didn't see a way
of doing it for workflow. So, the easiest thing (for now) might be to mark
it as a missing feature for Workflow jobs and not have that point block
this PR?

Then, separately have a discussion on the merits/demerits etc of
auto-removing triggers. This will either result in a task to remove the
feature completely (for all build types), or a task to find a way of making
it work for workflow.


Reply to this email directly or view it on GitHub
https://github.com/jenkinsci/gerrit-trigger-plugin/pull/240/files#r35523862
.

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2015

Member

Agreed with @tfennelly’s summary.

@jacob-keller

This comment has been minimized.

Copy link

commented on e67379d Jul 10, 2015

Didn't @rsandell suggest also to update the error to include the job name?

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Jul 11, 2015

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

Get the tests to compile
not sure if they pass though
@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2015

Didn't @rsandell suggest also to update the error to include the job name?

@jacob-keller Where's that?

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2015

The tests compile now, but there are failures galore :) Maybe you guys can help me make sense of these?

tfennelly added some commits Jul 13, 2015

Updated git-client plugin dep to v1.11.1
There's a newer version, but 1.11.1 is the version that git-server 2.3 has a dep on.
Added commons-io dep (scope=provided)
Why is this not coming in as a transitive dep of jenkins-core ?
if (project == null) {
project = Hudson.getInstance().getItemByFullName(projectId, AbstractProject.class);
project = Hudson.getInstance().getItemByFullName(projectId, Job.class);

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 11, 2015

Member

inherital sin.

@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

🐛 Found mostly just small nits.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2015

@rsandell I fixed what I reasonably could from your review. I don't think the others should hold up this PR so hopefully you can take back that bug please ;)

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2015

@rsandell and if you can point me to the SomethingApi that you are talking about then we can review that. I went through all changes and fixed backward compatibility wherever it was relevant, so there's nothing outstanding in this PR afaik.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2015

@rsandell oh and I purposely didn't touch/fix things like the use of Hudson instead of Jenkins. I fixed the ones you pointed to, but in general I do not consider that to be part of what this PR is about.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

🐝

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2015

@rsandell GerritTriggerApi didn't change.

@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

@tfennelly on the use of Hudson instead of Jenkins; This plugin contains too many old/bad practices for any one person to fix them all in a simple pull request. So we use the principle of inhetital sin i.e. if you touch a line you inherit all the sins of that line and gets asked to fix it, because they are now your sins :) That way we can hopefully inch our way to a better code base instead of giving up and never do anything about it.

That does not apply to for example bad architecture that spans over multiple lines and files though. I did not ask you for example to convert the rebuild actions to use a transient factory even though you've touched lines in relation to that ;)

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

@rsandell While I'm a person of zero religion/faith or supernatural beliefs of any kind now, I would have been born, baptised and educated within the Roman Catholic religion (as with most Irish people). So, I'm very familiar with sick ideas such as that of Ancestral/Original sin i.e. inheriting someone elses sins.

So while I always thought I had been "cleansed" at baptism, I now find my sole has been tarnished with your bad deeds. Is that how it works? Grrrrrrrr 😸

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

@rsandell anyway ... back to the main point ... this PR needs to move forward if we can 😄

@@ -1149,14 +1154,6 @@ public void testGerritEventManualEvent() {
trigger.createListener().gerritEvent(event);

verify(listener).onTriggered(same(project), same(event));

verify(project).scheduleBuild2(

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 14, 2015

Member

You are no longer verifying that the schedule2 method is actually called, the proxy above will only fail if it gets scheduled with bad parameters, but will pass the test if its not scheduled at all.

@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

Repent and thou shall be merged!

Sorry couldn't help myself 😈

Moving forward: I found some motivation to still not like the schedule thingie. 🐛 for removing the verify of schedule2. But I've gone through the rest and I it looks good.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

@rsandell fair point on the non-verify of the schedule2 call. Will fix that now. Thanks.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

@rsandell ok, that should be ok now (assuming CI tests pass + over-picky checkstyles).

@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

Sorry @tfennelly I was a bit lazy in my last comment.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

@rsandell added the <if>. Hopefully the tests all still pass coz not sure that instanceof check will work in the jelly.

As for the schedule, there may be a way of doing it differently by spying on the Queue, but I'd really not like to start into another round of dev on this PR for something that's not actually "broken" (i.e. we just dislike the current approach). This PR has now been sitting around for nearly a month (waiting mostly on you 😉). Is there any chance we can move these changes along and revisit the schedule issue to explore other ways of doing it.

The schedule method is protected, but I can also mark it with that @InternalUseOnly annotation (forget the exact name).

@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

But you did break the tests, they don't test the same thing any more.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

@rsandell specifically where are they not testing the same thing? You were saying that it was no longer verifying that the schedule method was bing called. That's fixed now. Where is the difference now?

rsandell added a commit that referenced this pull request Aug 15, 2015

Merge pull request #240 from tfennelly/JENKINS-26010
[JENKINS-26010] Workflow compatibility

@rsandell rsandell merged commit 5661bcf into jenkinsci:master Aug 15, 2015

1 check failed

Jenkins Looks like there's a problem with this pull request
Details
@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 15, 2015

I fixed the build failure and cleaned up the tests in 136d511 after the merge.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2015

@rsandell thanks for that Bobby. I'll take it for a drive and test it again.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.