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

Fix for JENKINS-24581 #1

Closed
wants to merge 7 commits into from

Conversation

pbthorste
Copy link
Contributor

These changes update the batch-task plugin so that it displays correctly in more recent versions of jenkins.

Added matrix-project plugin to satisfy the envinject lib which assumes
it is available.
Set the groovy version because of some build issues.
@jenkinsadmin
Copy link
Member

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

@@ -378,4 +381,10 @@ public Task getOwnerTask() {
public Object getSameNodeConstraint() {
return null;
}

@Override
public Authentication getDefaultAuthentication() {
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it I get the error:

The type BatchTask must implement the inherited abstract method Queue.Task.getDefaultAuthentication()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any chance you could take another look, daniel-beck ?

@pingbat
Copy link

pingbat commented Jun 17, 2015

Hello, I manually applied the patches from this PR and tried it out. The display is much better but starting tasks stopped working. My task seemed to be waiting for an executor to become available but nothing else was running.

@pbthorste
Copy link
Contributor Author

What version of Jenkins did you run the plugin on ? I have tested it on 1.615 and so far it has been working.

@pingbat
Copy link

pingbat commented Jun 26, 2015

I'm on the latest 1.617:

image

image

I may have made a mistake in applying the patches (I couldn't see an easy way to grab a ZIP file of the PR, I can't install GitHub for Windows)

I'll see if I can get the code onto a different machine and try it there.

@pingbat
Copy link

pingbat commented Jun 26, 2015

I've just checked my logs and I see this:

Jun 26, 2015 9:12:42 AM hudson.triggers.SafeTimerTask run
SEVERE: Timer task hudson.model.Queue$MaintainTask@cc1957 failed
java.lang.IllegalArgumentException: Authentication required
at org.springframework.util.Assert.notNull(Assert.java:112)
at org.acegisecurity.acls.sid.PrincipalSid.(PrincipalSid.java:45)
at hudson.security.SidACL._hasPermission(SidACL.java:70)
at hudson.security.SidACL.hasPermission(SidACL.java:52)
at hudson.model.Node.canTake(Node.java:366)
at hudson.model.Queue$JobOffer.canTake(Queue.java:296)
at hudson.model.Queue.maintain(Queue.java:1389)
at hudson.model.Queue$MaintainTask.doRun(Queue.java:2489)
at hudson.triggers.SafeTimerTask.run(SafeTimerTask.java:51)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
at java.util.concurrent.FutureTask$Sync.innerRunAndReset(FutureTask.java:351)
at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:178)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:178)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603)
at java.lang.Thread.run(Thread.java:722)

@pingbat
Copy link

pingbat commented Jun 29, 2015

I've changed BatchTask.getDefaultAuthentication() to this and it works for me now:

@OverRide
public Authentication getDefaultAuthentication() {
return ACL.SYSTEM;
}

@pingbat
Copy link

pingbat commented Sep 22, 2015

Any thoughts on merging this in with the ACL.SYSTEM change? I've been using it fine for months now.

@pbthorste
Copy link
Contributor Author

Sorry for taking so long - but the change is pushed

@michaelpaesold
Copy link
Contributor

We have been using a 1.18 snapshot build of the plugin based on the latest commit 3b0f9f4 in production successfully since end of 2015, running on Jenkins LTS versions 1.609.3 and later 1.625.3. We will upgrade to newer Jenkins versions soon.

I think this PR is good and should be merged. Are there any reasons not to do so, other than resolving conflicts?

The released version 1.19 does not display correctly on the Jenkins versions above.

@michaelpaesold
Copy link
Contributor

@pbthorste - would you update the PR? Otherwise I could take your branch and do that myself.

@michaelpaesold
Copy link
Contributor

I have created a new rebased PR #7 that only contains the actually required changes compared to current master.

@lacostej
Copy link
Contributor

lacostej commented Nov 2, 2023

Closing this as the code was merged in #12. Thanks @pbthorste

@lacostej lacostej closed this Nov 2, 2023
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.

6 participants