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

Jump to bottom

[FIXED JENKINS-41371][FIXED JENKINS-41948][FIXED JENKINS-41867] Update dependencies to Branch API 2.0.x #145

Merged
merged 5 commits into from Feb 15, 2017
Merged

[FIXED JENKINS-41371][FIXED JENKINS-41948][FIXED JENKINS-41867] Update dependencies to Branch API 2.0.x #145

merged 5 commits into from Feb 15, 2017

Conversation

stephenc
Copy link
Member

See JENKINS-41371 and JENKINS-41948

I have added some tests of indexing and event driven branch detection, the tests could be expanded to cover the other project types, but in principle these are sufficient to detect the two issues described.

I have added ignored tests for some important gaps with regards to the handling of dead branches

@reviewbybees

jglick
jglick suggested changes Feb 13, 2017
View reviewed changes
.../java/com/github/mjdetullio/jenkins/plugins/multibranch/FreeStyleMultiBranchProjectTest.java Outdated
"given_multibranchWithSources_when_branchRemoved_then_branchProjectIsNotBuildable",
SCMEvent.Type.REMOVED, c, "foo", "master", "junkHash"));
r.waitUntilNoActivity();
assertThat(master.getACL().hasPermission(ACL.SYSTEM, Item.BUILD), is(false));
Copy link
Member

Choose a reason for hiding this comment

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

As noted in jenkinsci/workflow-multibranch-plugin#53 this is pointless. The plugin should rather .setBuildable(false).

Copy link
Member Author

Choose a reason for hiding this comment

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

The next test does that. This is a proxy for the build action being visible

Copy link
Member

Choose a reason for hiding this comment

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

for the build action being visible

It will not be visible if !Job.buildable, so why not just delete this test case?

@stephenc
Copy link
Member Author

Latest build failure seems to be an infra issue:

[FATAL] Non-resolvable parent POM: Could not transfer artifact org.jenkins-ci.plugins:plugin:pom:2.21 from/to repo.jenkins-ci.org (http://repo.jenkins-ci.org/public/): Failed to transfer file: http://repo.jenkins-ci.org/public/org/jenkins-ci/plugins/plugin/2.21/plugin-2.21.pom. Return code is: 502 , ReasonPhrase:Bad Gateway. and 'parent.relativePath' points at wrong local POM @ line 9, column 13

	at org.apache.maven.project.DefaultProjectBuilder.build(DefaultProjectBuilder.java:364)
	at hudson.maven.MavenEmbedder.buildProjects(MavenEmbedder.java:361)
	at hudson.maven.MavenEmbedder.readProjects(MavenEmbedder.java:331)
	at hudson.maven.MavenModuleSetBuild$PomParser.invoke(MavenModuleSetBuild.java:1321)
	at hudson.maven.MavenModuleSetBuild$PomParser.invoke(MavenModuleSetBuild.java:1118)
	at hudson.FilePath$FileCallableWrapper.call(FilePath.java:2731)
	at hudson.remoting.UserRequest.perform(UserRequest.java:153)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:336)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(FutureTask.java:262)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
	at java.lang.Thread.run(Thread.java:745)

@stephenc stephenc closed this Feb 14, 2017
@stephenc stephenc reopened this Feb 14, 2017
@stephenc stephenc changed the title [FIXED JENKINS-41371][FIXED JENKINS-41948] Update dependencies to Branch API 2.0.x [FIXED JENKINS-41371][FIXED JENKINS-41948][FIXED JENKINS-41867] Update dependencies to Branch API 2.0.x Feb 14, 2017
mjdetullio
mjdetullio requested changes Feb 15, 2017
View reviewed changes
Copy link
Collaborator

@mjdetullio mjdetullio left a comment

Choose a reason for hiding this comment

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

Not sure I understand the why @Ignored tests are the way they are but I'll take your word for it. Will investigate some other time.

pom.xml
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>branch-api</artifactId>
<version>1.5</version>
<version>2.0.5</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Requires jenkins.version bump to 1.642.3

pom.xml Outdated
@@ -78,12 +78,12 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>cloudbees-folder</artifactId>
<version>5.7</version>
<version>5.17</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dependency can be removed instead, since branch-api 2.0.5 dictates this version

@mjdetullio mjdetullio merged commit 7ed1225 into jenkinsci:master Feb 15, 2017
@mjdetullio
Copy link
Collaborator

Thank you

@stephenc
Copy link
Member Author

@mjdetullio changes made.

w.r.t. the ignored tests, basically there is supposed to be a difference between Branch.Dead and a regular branch.

  • A Branch.Dead should not be buildable but should be deletable.
  • A regular branch should be buildable but should not be deletable.

As it currently stands, I do not think you can fix the buildable one without hacks.

The deletable one could perhaps be achieved with ACL decoration.

The only hack I can think of for buildable is to combine an ACL hack with a QueueDecisionHandler so that users cannot submit dead branches and if somehow they manage to "schedule" one then a QueueDecisionHandler will prevent them from executing. The correct fix will require some changes in core to make Job.isBuildable() consult the job properties.

Another thing I spotted is that you allow for the possibility of the branch being null because users may reconfigure. The fix for that is to store the branch in a side file.

@stephenc stephenc deleted the jenkins-41371-jenkins-41948 branch February 15, 2017 00:39
@stephenc
Copy link
Member Author

If you want to see an example of storing things in a side file see the https://github.com/jenkinsci/branch-api-plugin/blob/master/src/main/java/jenkins/branch/OrganizationFolder.java#L1305 State storage in Org folders. You could achieve something similar.

@jglick
Copy link
Member

jglick commented Feb 15, 2017

The correct fix will require some changes in core to make Job.isBuildable() consult the job properties.

Why not simply call AbstractProject.makeDisabled when the liveness status changes?

@stephenc
Copy link
Member Author

Why not simply call AbstractProject.makeDisabled when the liveness status changes?

OK, that would work, as there is already a hook to re-apply the configuration after every attempt by the user to save

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick approved these changes

@mjdetullio mjdetullio mjdetullio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@stephenc @mjdetullio @jglick