Fix Checkstyle errors and improve code quality (final parameters, lin… #1142
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,24 +28,37 @@ | |
| public class DockerQueueListener extends QueueListener { | ||
|
|
||
| @Override | ||
| public void onEnterWaiting(final WaitingItem wi) { | ||
| final DockerJobTemplateProperty jobTemplate = getJobTemplate(wi); | ||
| if (jobTemplate != null) { | ||
| final DockerCloud cloud = | ||
| DockerCloud.getCloudByName(jobTemplate.getCloudname()); | ||
| if (cloud != null) { | ||
| final String uuid = UUID.randomUUID().toString(); | ||
| final DockerTemplate template = | ||
|
Contributor
There was a problem hiding this comment. Please use |
||
| jobTemplate.getTemplate().cloneWithLabel(uuid); | ||
| cloud.addJobTemplate(wi.getId(), template); | ||
| wi.addAction(new DockerTemplateLabelAssignmentAction(uuid)); | ||
| } | ||
| } | ||
| } | ||
| /** | ||
| * Called when an item leaves the queue. | ||
| * Subclasses can override this method to provide custom behavior when an item is removed from the queue. | ||
| * | ||
| * <p>When overriding this method, ensure that you: | ||
| * 1. Call the superclass's `onLeft` method if needed, to preserve the default behavior. | ||
| * 2. Handle null checks for `jobTemplate` and `cloud` objects, as they can be `null`. | ||
| * 3. Avoid making any long-running or blocking operations in this method, as it is called within Jenkins' queue management thread. | ||
| * | ||
| * @param li the {@link LeftItem} that left the queue. This parameter should not be null. | ||
| */ | ||
| @Override | ||
| public void onLeft(final LeftItem li) { | ||
| final DockerJobTemplateProperty jobTemplate = getJobTemplate(li); | ||
| if (jobTemplate != null) { | ||
| final DockerCloud cloud = | ||
| DockerCloud.getCloudByName(jobTemplate.getCloudname()); | ||
| if (cloud != null) { | ||
| cloud.removeJobTemplate(li.getId()); | ||
| } | ||
|
|
@@ -59,23 +72,26 @@ public void onLeft(LeftItem li) { | |
| * @return If the item includes a template then the template will be returned. Otherwise <code>null</code>. | ||
| */ | ||
| @CheckForNull | ||
| private static DockerJobTemplateProperty getJobTemplate(final Item item) { | ||
| if (item.task instanceof Project) { | ||
| final Project<?, ?> project = (Project<?, ?>) item.task; | ||
| final DockerJobTemplateProperty p = project.getProperty( | ||
| DockerJobTemplateProperty.class); | ||
| if (p != null) { | ||
| return p; | ||
| } | ||
| // backward compatibility. DockerJobTemplateProperty used to be a nested object in DockerJobProperty | ||
| final DockerJobProperty property = project.getProperty( | ||
| DockerJobProperty.class); | ||
| if (property != null) { | ||
| return property.getDockerJobTemplate(); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static final class DockerTemplateLabelAssignmentAction extends InvisibleAction | ||
| implements LabelAssignmentAction { | ||
| private final String uuid; | ||
|
|
||
| private DockerTemplateLabelAssignmentAction(String uuid) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,7 +172,7 @@ public DockerComputer createComputer() { | |
|
|
||
| @Nullable | ||
| @Override | ||
| public final ProvisioningActivity.Id getId() { | ||
| return provisioningId; | ||
| } | ||
|
|
||
|
|
@@ -184,7 +184,7 @@ private interface ILogger { | |
|
|
||
| @Override | ||
| @Restricted(NoExternalUse.class) | ||
| public final void _terminate(final TaskListener listener) { | ||
| final ILogger tl = createILoggerForTaskListener(listener); | ||
| try { | ||
| terminate(tl); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,7 +186,7 @@ enum ArgumentVariables { | |
| private final String name; | ||
| private final String description; | ||
|
|
||
| ArgumentVariables(final String name, final String description) { | ||
| this.name = name; | ||
| this.description = description; | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /** | ||
| * This package contains classes related to the Jenkins pipeline integration | ||
| * for Docker, including the management of Docker nodes and steps within the pipeline. | ||
| * | ||
| * <p>The classes in this package manage Docker pipeline steps, node provisioning, | ||
| * and the execution of build jobs within Docker containers.</p> | ||
| */ | ||
| package io.jenkins.docker; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,7 +186,7 @@ private static DockerAPI defaultApi() { | |
| "Must either specify dockerHost/credentialsId, or define at least one Docker cloud"); | ||
| } | ||
|
|
||
| private void invokeBody(final DockerTransientNode node, final TaskListener listener) { | ||
| this.nodeName = node.getNodeName(); | ||
| FilePath ws = null; | ||
| Computer computer = null; | ||
|
|
@@ -256,7 +256,7 @@ public void stop(@NonNull Throwable cause) throws Exception { | |
| private static class Callback extends BodyExecutionCallback.TailCall { | ||
| private final String nodeName; | ||
|
|
||
| Callback(Node node) { | ||
| this.nodeName = node.getNodeName(); | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Please use the most recent release of the checkstyle plugin.
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.
Thank you for your previous comments and guidance,
I noticed that we don't currently have a checkstyle.xml file in the repository or a clear indication of where it might be referenced. I'm wondering if there is a plan to introduce this file, or if we're using an external/shared configuration that I might have overlooked? I ran mvn spotless:apply successfully, but just wanted to make sure we're not missing any custom style rules that might come with Checkstyle.
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.
I'm not planning to introduce checkstyle into this repository. The Jenkins plugins that I maintain do not use checkstyle. I've not found it to be valuable enough to be worth the effort to enable it.
If you're serious about enabling checkstyle in this repository, then you'll need to provide a complete implementation that includes the checkstyle settings to persuade me that it will be useful. The default checkstyle settings conflict with spotless formatting. I value spotless formatting much more than I value checkstyle warnings. If your pull request proposes to accept many new, unresolved checkstyle warnings, then I'll reject it. I'm not willing to add new warnings output because it will be a distraction and will create extra work for me to resolve the warnings.