[JENKINS-43900] Update to 2.X parent POM #5

varyvol · 2017-04-27T12:59:09Z

Update to 2.X parent POM.
Bump Jenkins and plugins versions.
Fix Findbugs errors.

@reviewbybees @olamy @stephenc @jglick @kwhetstone @andresrc

oleg-nenashev

🐛 for getType() check. It is required to ensure it was not a migration logic before decoupling of the plugin

oleg-nenashev · 2017-04-27T13:02:26Z

src/findbugs/excludesFilter.xml

@@ -25,4 +25,16 @@
  <Match>
    <Class name="~.*\.Messages"/>
  </Match>
+  <Match>
+    <Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>


Please use @SuppressFBWarnings() instead

Changed but, out of curiosity, why is that preferable?

So that you can write a clear justification in every case.

oleg-nenashev · 2017-04-27T13:06:51Z

src/main/java/com/cloudbees/plugins/deployer/DeployNowRunAction.java

-        } else {
-            return new InputStreamReader(getLogInputStream(), getCharset());
-        }
+        return new InputStreamReader(getLogInputStream(), getCharset());


Not sure if it is correct, but OK

getCharset() is already checking for nullability and returning default charset in that case.

oleg-nenashev · 2017-04-27T13:09:00Z

src/main/java/com/cloudbees/plugins/deployer/sources/MavenArtifactDeploySource.java

@@ -158,7 +158,7 @@ public String getGroupId() {
     */
    @NonNull
    public String getType() {
-        return type == null ? DEFAULT_TYPE : type;
+        return type;


🐛 I am pretty sure it was a migration logic. Should be replaced by readResolve

I can be missing something, but type is being set in the constructor, where the null check is already done.

Object deserialization does not involve the constructor. So, if somebody migrates old data, this field can be probably null

Right so I am reverting this. Thanks @oleg-nenashev

ghost · 2017-04-27T13:53:35Z

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.

jglick · 2017-05-03T17:54:14Z

src/main/java/com/cloudbees/plugins/deployer/DeployEvent.java

@@ -36,6 +37,7 @@
 /**
 * @author stephenc
 */
+@SuppressFBWarnings(value = "SE_BAD_FIELD")


Why? This certainly looks like a bug to me. AbstractBuild cannot be serialized. So either make the field transient, or remove implements Serializable if no one is actually serializing it, or explain why this code is not doing what it looks like.

Removed Serializable as it would not be working up to now.

oleg-nenashev · 2017-05-05T13:06:39Z

src/main/java/com/cloudbees/plugins/deployer/DeployEvent.java

 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;

 /**
 * @author stephenc
 */
-public class DeployEvent implements Serializable {
+public class DeployEvent {


Needs a weight-in from the maintainer. The class is definitely not serializable in the current implementation. But removal of the interface breaks binary compatibility.

I guess nobody was serializing this because there would be issues raised. But a confirmation from the maintainer would be great for sure.

CC @olamy , @stephenc and @christ66

I'm in agreement with @oleg-nenashev . Even though removing it is harmless it would break binary compatibility.

@christ66 so then supressing back the Findbugs error?

just remove the interface

Yeah if it is not actually serializable, then removing the interface does not break anything that was not already broken.

I will leave it to @christ66, @stephenc and @olamy since they are the plugin maintainers. There is a way to make the class serializable, but I am not sure it worth effort (though I would really vote for it if I was the maintainer)

oleg-nenashev · 2017-05-05T13:44:30Z

src/main/java/com/cloudbees/plugins/deployer/sources/MavenArtifactDeploySource.java

@@ -157,6 +158,7 @@ public String getGroupId() {
     * @return the type of artifact to match.
     */
    @NonNull
+    @SuppressFBWarnings(value = "RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE", justification = "Might be null if serialized")


"deserialized", but ok

varyvol · 2017-05-15T10:10:27Z

@olamy @stephenc @christ66 could you please review this?

stephenc

looks ok to me... seems like a lot of fussing over nothing... though I have given up maintaining this plugin as E_TO_MANY_PLUGINS

varyvol · 2017-05-25T14:20:50Z

@oleg-nenashev @jglick @christ66 @olamy once Stephen has approved this, are you fine with this getting merged?

jglick

Incorrect use of Remoting.

jglick · 2017-05-25T14:36:51Z

src/main/java/com/cloudbees/plugins/deployer/DeployEvent.java

 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;

 /**
 * @author stephenc
 */
-public class DeployEvent implements Serializable {
+public class DeployEvent {


Yeah if it is not actually serializable, then removing the interface does not break anything that was not already broken.

jglick · 2017-05-25T14:38:02Z

src/main/java/com/cloudbees/plugins/deployer/MavenInstallationProvider.java

@@ -36,7 +36,7 @@
 /**
 * Since we allow token expansion, we must provide a default Maven path -- in
 * case they are running on a master without any Mavens.
- * <p/>
+ * <p></p>


This is actually wrong. Just use

<p>

jglick · 2017-05-25T14:52:04Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

@@ -232,7 +234,7 @@ public void log(String message) {
        throw new DeployException("Deployment hosts of type " + configuration.getClass() + " are unsupported");
    }

-    public static class FingerprintDecorator implements Callable<DeployedApplicationLocation, IOException> {
+    public static class FingerprintDecorator extends MasterToSlaveCallable<DeployedApplicationLocation, IOException> {


I do not think this is true. FingerprintingWrapper has code which apparently calls it from the agent side, if that callable was running remotely. And its body calls Hudson.getInstance() without a null check, confirming that it is intended to be run on the master side. Thus it should be a SlaveToMasterCallable.

But before you make such a change you would need to review its implementation carefully to ensure that no malicious arguments could possibly cause any harm. That seems unlikely, since the agent could pass any md5sum it likes, mangling fingerprints of some unrelated artifact. Which means that really FingerprintDecorator needs to be deleted, and FingerprintingWrapper needs to be reworked so that any information about fingerprints is simply returned, to be handled by its caller (after any necessary sanity checks). That in turn means understanding the call points in the two Engine.process overloads. The one taking FilePath is called from perform in what is clearly a master context (because it is calling build.getWorkspace(), and a Run can only exist on master); thus the other overload must also be called from master. Thus it is the process methods which would need to add suitable DeployedApplicationFingerprintFacets; perhaps FingerprintingWrapper needs to be returning a pair of DeployedApplicationLocation and md5sum.

@jglick So let's see if I follow you. You mean that:

The engine.process() is executed in master.

It calls the wrapper, which is executed in the agent.

This one calls the decorator, which is executed in the master.

Again in the wrapper, eventually it returns this information to the master.

And you think such "travel" to decorate could be avoided by returning what's necessary in the wrapper and just using it in the process() method. Is that it?

If so, what it's not clear to me is if you think this is something to improve or something that you think that is required. Because if the first one, I would prefer to tackle that in another PR to move this forward and not to increase the size of this one.

Is that it?

Yes.

if you think this is something to improve or something that you think that is required

I think fixing this is required. Certainly the PR as it currently stands is incorrect. Anyway it should be a pretty small refactoring; I am happy to offer a suggested commit if you are unsure.

Last commit should make it. Please let me know if you think something is not appropriate.

jglick · 2017-05-25T14:53:00Z

src/main/java/com/cloudbees/plugins/deployer/resolvers/PromotionCapabilitiesResolver.java

@@ -119,7 +119,10 @@ protected boolean knows(@NonNull Class<? extends AbstractProject> jobType) {
     */
    @Override
    public Run<?, ?> getLastSuccessfulBuild(AbstractProject<?, ?> project) {
-        return CapabilitiesResolver.getLastDeployableBuild(PromotionProcess.class.cast(project).getRootProject());
+        if (project != null) {
+            return CapabilitiesResolver.getLastDeployableBuild(PromotionProcess.class.cast(project).getRootProject());


Pointless use of Class.cast (should never be used with a constant receiver). Just

(PromotionProcess) project

… slave.

jglick

Much better, thanks!

@stephenc you might want to take a look since this is your code IIRC

jglick · 2017-06-14T16:45:51Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

@@ -205,6 +206,33 @@ public DeployedApplicationLocation process(File applicationFile, T target) throw
        }
    }

+    private DeployedApplicationLocation addToFacects(AbstractMap.SimpleEntry<String, DeployedApplicationLocation> pair) throws IOException {


jglick · 2017-06-14T16:46:35Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

-    }
-
-    public static class FingerprintingWrapper extends MasterToSlaveFileCallable<DeployedApplicationLocation> {
+    public static class FingerprintingWrapper extends MasterToSlaveFileCallable<AbstractMap.SimpleEntry<String, DeployedApplicationLocation>> {


clearer to use Map.Entry<…> as the declared type

jglick · 2017-06-14T16:47:00Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

@@ -181,7 +180,8 @@ public boolean perform() throws Throwable {
    public DeployedApplicationLocation process(FilePath applicationFile, T target) throws DeployException {
        try {
            FingerprintingWrapper actor = new FingerprintingWrapper(newDeployActor(target), listener);
-            return applicationFile.act(actor);
+            AbstractMap.SimpleEntry<String, DeployedApplicationLocation> pair = applicationFile.act(actor);
+            return this.addToFacects(pair);


arguably more readable if pair is inlined (so the verbose type need not be mentioned)

Absolutely! Changed.

jglick · 2017-06-14T16:47:52Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

+
+        if (!NO_MD5.equals(md5sum)) {
+            Jenkins j = Jenkins.getInstance();
+            if (j == null) {


can use getActiveInstance in suitable core versions (later it is back to getInstance)

Not available in 1.580.1

jglick · 2017-06-14T16:49:11Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

-                    if (instance != null) {
-                        return decorator.call();
-                    } else {
-                        Channel current = Channel.current();


Any deletion of a call to Channel.current() is a happy day!

jglick · 2017-06-14T16:49:53Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

                } catch (Exception e) {
                    listener.error("[cloudbees-deployer] Could not record fingerprint association");
-                    // e.printStackTrace(listener.getLogger());
                } catch (LinkageError e) {


This is very possibly obsolete.

jglick · 2017-06-14T16:51:11Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

-                                : "Can only have a null Jenkins instance if on the other end of a channel";
-                        return current.call(decorator);
-                    }
+                    return new AbstractMap.SimpleEntry<String, DeployedApplicationLocation>(md5sum, location);
                } catch (Exception e) {


The try-catch could probably be deleted and the caller made to handle any of the usual exceptions, as it probably already is. I suspect this was here in case the callback to master failed, which is now an obsolete condition.

Yes, I have removed them as they are being captured in the process() method.

Requested change was made AFAICT.

…nused code.

jglick · 2017-06-16T11:11:41Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

+                    String md5sum = Util.getDigestOf(fis);
+                    return new AbstractMap.SimpleEntry<String, DeployedApplicationLocation>(md5sum, location);
+                } finally {
+                    IOUtils.closeQuietly(fis);


just

fis.close();

suffices

varyvol · 2017-06-19T06:50:53Z

@oleg-nenashev @stephenc @christ66 any extra comment here?

oleg-nenashev · 2017-06-20T15:27:13Z

src/main/java/com/cloudbees/plugins/deployer/engines/Engine.java

@@ -232,76 +261,26 @@ public void log(String message) {
        throw new DeployException("Deployment hosts of type " + configuration.getClass() + " are unsupported");
    }

-    public static class FingerprintDecorator implements Callable<DeployedApplicationLocation, IOException> {


Also incompatible

Allows us to delete this nonsense.

oleg-nenashev

I am not ready to approve it due to the incompatible changes like FingerprintDecorator class removal.

But I leave the decision to maintainers. If the current version gets merged, I just strongly recommend to release it as a new major version (2.0)

jglick · 2017-06-20T15:37:19Z

I do not see any incompatibilities here and see no need for a 2.0 version.

stephenc

Seems ok, but I haven't looked at this plugin in ages

stephenc · 2017-06-23T13:02:07Z

src/main/java/com/cloudbees/plugins/deployer/sources/MavenArtifactDeploySource.java

@@ -157,6 +158,7 @@ public String getGroupId() {
     * @return the type of artifact to match.
     */
    @NonNull
+    @SuppressFBWarnings(value = "RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE", justification = "Might be null if deserialized")
    public String getType() {
        return type == null ? DEFAULT_TYPE : type;


Could remove the fb warning suppression with StringUtils.defaultString(type, DEFAULT_TYPE)

varyvol · 2017-06-23T13:18:39Z

@stephenc thanks.

@olamy @jglick @oleg-nenashev do you think this could be merged (and released) now?

oleg-nenashev · 2017-06-23T13:38:00Z

@varyvol You can see my feedback above. If I was the plugin maintainer, then I would request changes. But I am not, hence it's up to its maintainers. @reviewbybees done according to the votes

The ball is on the side of @stephenc and @olamy (according to pom.xml)

varyvol · 2017-06-23T13:49:33Z

@oleg-nenashev why do you think the changes are incompatible? The class is used only internally, and it's behaviour has been maintained.

oleg-nenashev · 2017-06-23T13:54:16Z

@varyvol

why do you think the changes are incompatible? The class is used only internally, and it's behaviour has been maintained.

FingerprintDecorator class is public. Deletion of public classes is always formally incompatible, because it is not possible to guarantee that nobody uses them (e.g. in custom System Groovy scripts or private-source plugins like CloudBees ones)

jglick · 2017-06-23T19:23:30Z

it is not possible to guarantee that nobody uses them

Of course not, but there is no remotely plausible use case for this class outside its plugin.

varyvol · 2017-06-27T06:59:46Z

So is there anything I can do to move this forward?

jglick · 2017-06-28T18:36:59Z

I think @stephenc is the maintainer.

varyvol · 2017-07-10T07:57:11Z

@stephenc ping. Could you please review (and merge if appropriate) this?

oleg-nenashev · 2017-07-12T09:13:42Z

@reviewbybees done

jglick · 2017-10-04T20:59:41Z

@stephenc again

stephenc · 2017-10-04T21:19:55Z

@jglick can someone else roll the release or do you need me to?

daniel-beck · 2018-08-22T10:13:47Z

Any intentions to release this?

oleg-nenashev · 2019-06-04T22:22:22Z

I'd guess it will happen soon after #7

Evaristo Gutiérrez added 2 commits April 27, 2017 14:53

[JENKINS-43900] Update to 2.X parent POM.

a332b1c

[JENKINS-43900] Fix findbugs errors.

9e268da

oleg-nenashev previously requested changes Apr 27, 2017

View reviewed changes

[JENKINS-43900] Use annotation instead of filter for Findbugs.

5b3d95a

jglick suggested changes May 3, 2017

View reviewed changes

[JENKINS-43900] Remove serializable from class that is not.

0a1c77a

oleg-nenashev reviewed May 5, 2017

View reviewed changes

Evaristo Gutiérrez added 2 commits May 5, 2017 15:25

[JENKINS-43900] Revert nullability checks.

ac2ffe2

[JENKINS-43900] Suppress fb warnings.

04366f8

oleg-nenashev reviewed May 5, 2017

View reviewed changes

varyvol closed this May 5, 2017

varyvol reopened this May 5, 2017

stephenc approved these changes May 19, 2017

View reviewed changes

jglick suggested changes May 25, 2017

View reviewed changes

[JENKINS-43900] Simplify logic by reducing “traveling” from master to…

5088b6d

… slave.

jglick approved these changes Jun 14, 2017

View reviewed changes

Evaristo Gutiérrez added 2 commits June 16, 2017 09:34

[JENKINS-43900] Address PR comments.

d697a1a

[JENKINS-43900] Get back the closure of the input stream and remove u…

59d35e9

…nused code.

jglick approved these changes Jun 16, 2017

View reviewed changes

[JENKINS-43900] Close directly the input stream.

651485f

jglick approved these changes Jun 16, 2017

View reviewed changes

andresrc approved these changes Jun 20, 2017

View reviewed changes

oleg-nenashev reviewed Jun 20, 2017

View reviewed changes

stephenc reviewed Jun 23, 2017

View reviewed changes

[JENKINS-43900] Avoid FB suppression.

916da5f

christ66 approved these changes Jun 28, 2017

View reviewed changes

stephenc approved these changes Oct 4, 2017

View reviewed changes

stephenc merged commit dda2f2c into jenkinsci:master Oct 4, 2017

[JENKINS-43900] Update to 2.X parent POM #5

[JENKINS-43900] Update to 2.X parent POM #5

Conversation

varyvol commented Apr 27, 2017

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyvol commented May 15, 2017

stephenc left a comment

Choose a reason for hiding this comment

varyvol commented May 25, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyvol commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jglick commented Jun 20, 2017

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varyvol commented Jun 23, 2017

oleg-nenashev commented Jun 23, 2017

varyvol commented Jun 23, 2017

oleg-nenashev commented Jun 23, 2017 • edited

jglick commented Jun 23, 2017 • edited

varyvol commented Jun 27, 2017

jglick commented Jun 28, 2017

varyvol commented Jul 10, 2017

oleg-nenashev commented Jul 12, 2017

jglick commented Oct 4, 2017

stephenc commented Oct 4, 2017

daniel-beck commented Aug 22, 2018

oleg-nenashev commented Jun 4, 2019

oleg-nenashev commented Jun 23, 2017 •

edited

jglick commented Jun 23, 2017 •

edited