-
Notifications
You must be signed in to change notification settings - Fork 184
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-36479] First work on auto-clearing invalid locks. #35
Conversation
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
@@ -1,6 +1,5 @@ | |||
package org.jenkins.plugins.lockableresources; | |||
|
|||
import java.io.IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try
git checkout master -- src/main/java/org/jenkins/plugins/lockableresources/LockStepExecution.java
to revert irrelevant changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I went on the warpath of removing unused imports while I was here.
@amuniz had some reason for using two sets of APIs: one for |
Update LockRunListener to work with Runs rather than just AbstractProjects - this means that onCompleted and onDeleted will fire correctly, obviating the need for anything more special-case/complex.
Rewrote the commit and force-pushed it. |
I want to keep the listener for non-Pipelines jobs, where the lock flow is different as it applies to the whole build execution (you can not lock/unlock more than one resource at specific timing during a build), however that's possible in a Pipeline. Looking at this specific change I'd say that About APIs in |
|
||
static final String LOG_PREFIX = "[lockable-resources]"; | ||
static final Logger LOGGER = Logger.getLogger(LockRunListener.class | ||
.getName()); | ||
|
||
@Override | ||
public void onStarted(AbstractBuild<?, ?> build, TaskListener listener) { | ||
public void onStarted(Run<?, ?> build, TaskListener listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Nothing to do for non-AbstractBuild
s here, a check is needed to just return
in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this around to just return
if the build isn't an AbstractBuild
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I kept the signature as Run
for consistency with the onDeleted
and onCompleted
methods - I'd rather the consistent signature and handling of the AbstractBuild
case than inconsistent signatures. But I'm flexible. =)
@amuniz So - what's your recommendation? Just switch out |
@amuniz So I think it does make sense to keep |
@@ -61,7 +61,7 @@ public LockableResourcesManager() { | |||
return matching; | |||
} | |||
|
|||
public List<LockableResource> getResourcesFromBuild(AbstractBuild<?, ?> build) { | |||
public List<LockableResource> getResourcesFromBuild(Run<?, ?> build) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amuniz I know this is changing the API here and you did deliberately keep them separate, but given that this doesn't have side effects and works exactly the same whether you pass it an AbstractBuild
or a Run
, I'm not sure I see the reason to have it just be for AbstractBuild
s, especially since the only places it's called are in LockRunListener#onCompleted
and LockRunListener#onDeleted
, which now do need to operate on Run
s...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Right. Current status LGTM 🐝 and 👍 |
🐝 |
// Skip locking for multiple configuration projects, | ||
// only the child jobs will actually lock resources. | ||
if (build instanceof MatrixBuild) | ||
return; | ||
|
||
AbstractProject<?, ?> proj = Utils.getProject(build); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could have produced a much shorter diff using
if (!(build instanceof AbstractBuild)) {
return;
}
// …as before
Belated 🐝 |
JENKINS-36479
Update LockRunListener to work with Runs rather than just
AbstractProjects - this means that onCompleted and onDeleted will fire
correctly, obviating the need for anything more special-case/complex.
Probably supersedes #34 - this is a more general case solution that doesn't require new lock requests to clear the defunct locks.
cc @reviewbybees, esp @jglick and @amuniz