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

[FIXED JENKINS-27651] handle job rename #218

Merged
merged 5 commits into from Apr 1, 2015

Conversation

Projects
None yet
6 participants
@rsandell

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

Looks good, some unit test would be nice though.

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

Seems right though a test would be in order.

BTW please include a link to the JIRA issue in the PR description, and use the Web Link option in JIRA to create a link back to the PR, assigning to yourself and marking the issue In Progress.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

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

GerritItemListener listener = new GerritItemListener();
listener.onLocationChanged(job, "MyJob", "MyJobRenamed");
PluginImpl.getInstance().getHandler().notifyListeners(event);
Thread.sleep(100);

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 28, 2015

Author Member

is there a better way to give the job some time to build ?
Just need to check it has been scheduled.

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 28, 2015

Member

Jenkins.getInstance().getQueue().maintain()

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 29, 2015

Author Member

doesn't work. Test pass with Thread.sleep(100), but fails with Jenkins.getInstance().getQueue().maintain();

@ndeloof

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2015

build failure for Checkstyle violations :-\

conform to useless incredibly strict check style rules.
Break the build because javadoc first line is missing a dot, really ?
for (GerritProject p : gerritProjects) {
GerritProjectList.addProject(p, this);
}
}

This comment has been minimized.

Copy link
@jglick

jglick Mar 30, 2015

Member

Pointless diff hunk, revert.

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 30, 2015

Author Member

This is required by checkstyle rules on project, or maven install fails

GerritItemListener listener = new GerritItemListener();
listener.onLocationChanged(job, "MyJob", "MyJobRenamed");
PluginImpl.getInstance().getHandler().notifyListeners(event);
Jenkins.getInstance().getQueue().maintain();

This comment has been minimized.

Copy link
@jglick

jglick Mar 30, 2015

Member

Did you mean to use JenkinsRule.waitUntilNoActivity (CC @stephenc)?

}

/** some delay to ensure the job is ran after being scheduled. */
public static final int DELAY = 100;

This comment has been minimized.

Copy link
@jglick

jglick Mar 30, 2015

Member

This should not be necessary.

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 30, 2015

Member

Checkstyle complains about any numeric literals above 1 But in test code its OK to add

//CS IGNORE MagicNumber FOR NEXT 100 LINES. REASON: Test code

Simpler than to have a separate set of checkstyle rules for test code and production.


when(trigger.isInteresting(event)).thenReturn(true);
job.renameTo("MyJobRenamed");
GerritItemListener listener = new GerritItemListener();

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 30, 2015

Member

Since the JenkinsRule is part of the test I doubt you need to create a new listener and call onLocationChanged on it, job.renameTo should do that, right?

This comment has been minimized.

Copy link
@ndeloof

ndeloof Mar 30, 2015

Author Member

indeed.

listener.onLocationChanged(job, "MyJob", "MyJobRenamed");
PluginImpl.getInstance().getHandler().notifyListeners(event);
Jenkins.getInstance().getQueue().maintain();
Thread.sleep(DELAY);

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 30, 2015

Member

TestUtils.waitForBuilds(job, 1);

job.renameTo("MyJobRenamed");
PluginImpl.getInstance().getHandler().notifyListeners(event);
j.waitUntilNoActivity();
Assert.assertNotNull(job.getLastBuild());

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 30, 2015

Member

You should also test that the old listener isn't there any more, otherwise we would be leaking memory for every rename.

@rsandell

This comment has been minimized.

Copy link
Member

commented Mar 31, 2015

BackCompat252HudsonTest is flaky at times causing the build to timeout and caused the build failure.

@rsandell

This comment has been minimized.

Copy link
Member

commented Mar 31, 2015

@TWestling this seems to belong in 2.13.0 so I'm thinking about merging this and release a new beta, together with maybe some other bugfixes. WDYT?

@TWestling

This comment has been minimized.

Copy link
Member

commented Apr 1, 2015

@rsandell I agree, sounds good.

rsandell added a commit that referenced this pull request Apr 1, 2015

Merge pull request #218 from ndeloof/master
[FIXED JENKINS-27651] handle job rename

@rsandell rsandell merged commit 80e0abb into jenkinsci:master Apr 1, 2015

1 check failed

Jenkins
Details
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.