Update pom structure #48

mezpahlan · 2018-04-15T21:35:17Z

The current POM structure is quite out of date to what you get when creating an up to date Hello World plugin from the tutorial web pages. This PR updates the structure and versions of the parent POM.

Along with being aligned to the new structure we also resolve Enforcer and Findbug errors that come as a result of the newer Jenkins plugin dependencies.

The overall aim was to add support for a friendly syntax whilst using this plugin in pipeline mode. Getting the POM up to date seems a good first step to that.

Based on a fresh plugin project started with https://jenkins.io/doc/developer/tutorial/create/

Corrects Enforcer rule RequireUpperBoundDeps.

Resolves Findbugs URF_UNREAD_FIELD

Resolves Findbugs NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

Resolves Findbugs RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE

Resolves Findbugs DM_DEFAULT_ENCODING

Resolves Findbugs REC_CATCH_EXCEPTION

Resolves Findbugs NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

Resolves Findbugs RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

Resolves Findbugs OBL_UNSATISFIED_OBLIGATION_EXCEPTION_EDGE

Accidentally removed too many fields in previous commits and forgot to remove data bound constructor params in another previous commit.

oleg-nenashev

Note that there is already #45, which facelifts the plugin without breaking changes and reformatting. It does not fix FindBugs tho

oleg-nenashev · 2018-04-15T22:22:04Z

src/main/java/hockeyapp/HockeyappApplication.java


    @DataBoundConstructor
-    public HockeyappApplication(String apiToken, String appId, boolean notifyTeam,
+    public HockeyappApplication(String apiToken, boolean notifyTeam,


breaks binary compatibility

mezpahlan · 2018-04-15T22:47:25Z

I see now. Yes, you're quite right. Apologies. (First time contributing to Jenkins, forgive me).

What would you suggest with regards to this PR? Undo the charges that break binary compatibility, undo changes that duplicate [JENKINS-48949] - Make the plugin testable in PCT with JEP-200 #45, leave the Findbugs fixes? Something else?
With regards to that breaks binary compatibility comment: Findbugs complains that the field the parameter sets is unused. So removing the field to make Findbugs happy surely means removing the parameter?

Anyways, thanks for looking at this.

oleg-nenashev · 2018-04-16T07:05:22Z

@mezpahlan no problem at all, it happens. Generally we need a plugin maintainer to review and integrate something. I would not mind if your PR gets integrated instead of mine. In #45 I have also replaced bundled libs by Apache HttpComponents Client 4.x API Plugin, but it can be done in a follow-up if needed.

I am not sure who is the current plugin maintainer, @Brantone @dkochetkov and @ohoeltke are listed in https://github.com/jenkins-infra/repository-permissions-updater/blob/master/permissions/plugin-hockeyapp.yml

With regards to that breaks binary compatibility comment: Findbugs complains that the field the parameter sets is unused. So removing the field to make Findbugs happy surely means removing the parameter?

The field is public, so it may be used somehow. My recommendation would be to just use @SupressFBWarnings with a "TODO" comment for now

Add a suppression to Findbugs with a follow up TODO comment.

mezpahlan · 2018-04-16T19:30:28Z

Thanks I've reverted that binary breaking change and added your suggestions. Hopefully one of the maintainers can also chime in and move this forward. I'd love to help out with further improvements to this plugin in the future too. Happy to help where I can.

oleg-nenashev · 2018-05-01T08:34:27Z

pom.xml

+    <properties>
+        <jenkins.version>1.609.2</jenkins.version>
+        <java.level>7</java.level>
+        <jenkins-test-harness.version>2.38</jenkins-test-harness.version>


I would suggest to remove it and let O=PluginPOM use it's default one

oleg-nenashev · 2018-05-01T08:35:33Z

pom.xml

+    <packaging>hpi</packaging>
+
+    <properties>
+        <jenkins.version>1.609.2</jenkins.version>


If you require Java 7, it's better to update to Jenkins 1.625.3. Just because 1.609.x can run on Java 6 and hence the plugin may not work there

oleg-nenashev · 2018-05-01T08:37:10Z

src/main/java/hockeyapp/HockeyappApplication.java

+    @SuppressFBWarnings(value = {"URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"}, justification = "Breaks binary compatibility if removed.")
    @XStreamAsAttribute
-    public long schemaVersion;
+    public long schemaVersion; // TODO: Fix Findbugs gracefully.


Should it be marked as deprecated then? Just in case thee is really no usages anywhere (including Jelly and Co)

oleg-nenashev · 2018-05-01T08:38:10Z

src/main/java/hockeyapp/HockeyappApplication.java

-                    return FormValidation.errorWithMarkup(
-                            "You must enter an <a href=\"https://rink.hockeyapp.net/manage/auth_tokens\">API Token</a>.");
+            if (value.isEmpty()) {
+                final Jenkins instance = Jenkins.getInstance();


in 1.625.3 you can use getActiveInstance() IIRC. Jenkins instance cannot be really null here

oleg-nenashev · 2018-05-01T08:39:12Z

src/main/java/hockeyapp/HockeyappRecorder.java


-    private static final Charset UTF8_CHARSET = Charset.forName("UTF-8");
+    private static final String UTF8 = "UTF-8";
+    private static final Charset UTF8_CHARSET = Charset.forName(UTF8);


In Java 7 you can use StandardCharsets.

oleg-nenashev · 2018-05-01T08:41:27Z

pom.xml

+        <dependency>
+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpclient</artifactId>
+            <version>4.5.2</version>


Once the core is updated to 1.625.3, you could use Apache HttpComponents Client 4.x API plugin instead

oleg-nenashev · 2018-05-01T08:42:17Z

pom.xml

+        </dependency>
+        <dependency>
+            <groupId>commons-codec</groupId>
+            <artifactId>commons-codec</artifactId>


Why don't you just use the version from the core?

Uses the jenkins-test-harness version defined in the parent POM.

We require Java 7 so 1.625.3 is a better fit for that.

These are non-null by design.

We can let someone else manage Apache HttpComponents for us :)

This is inherited from the parent anyway. It also looks wrong.

Appears to be unused in this project.

oleg-nenashev · 2018-05-25T01:17:50Z

pom.xml

+    </developers>
+
+
+    <url>http://wiki.jenkins-ci.org/display/JENKINS/HockeyApp+Plugin</url>


https://wiki.jenkins.io/....

# Conflicts: # pom.xml

mezpahlan added 11 commits April 15, 2018 16:58

Migrate to current POM

6455643

Based on a fresh plugin project started with https://jenkins.io/doc/developer/tutorial/create/

Update dependency versions

46331e3

Corrects Enforcer rule RequireUpperBoundDeps.

Remove unread fields

dc78f76

Resolves Findbugs URF_UNREAD_FIELD

Guard against possible null pointers

27828ad

Resolves Findbugs NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

Remove redundant null check

56d303a

Resolves Findbugs RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE

Provide encoding to streams explicitly

059657d

Resolves Findbugs DM_DEFAULT_ENCODING

Explicitly catch thrown exceptions

0acf304

Resolves Findbugs REC_CATCH_EXCEPTION

Guard against null pointer

edbf0d4

Resolves Findbugs NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

Use return value rather than ignoring

398d820

Resolves Findbugs RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

Use try-with-resources to auto close stream

79ae9d3

Resolves Findbugs OBL_UNSATISFIED_OBLIGATION_EXCEPTION_EDGE

Clean up Bean class

98a5fdc

Accidentally removed too many fields in previous commits and forgot to remove data bound constructor params in another previous commit.

oleg-nenashev reviewed Apr 15, 2018

View reviewed changes

Restore previously removed public fields

f2ad5da

Add a suppression to Findbugs with a follow up TODO comment.

oleg-nenashev reviewed May 1, 2018

View reviewed changes

mezpahlan added 8 commits May 1, 2018 20:23

Remove explicitly overridden property

047a4cb

Uses the jenkins-test-harness version defined in the parent POM.

Bump Jenkins version

026c1fb

We require Java 7 so 1.625.3 is a better fit for that.

Mark unused public fields as deprecated

ce4a6b2

Migrate instance accessors to getActiveInstance

ed7cc3e

These are non-null by design.

Migrate to using standard Java 7 character sets

a70b2ea

Migrate to apache-httpcomponents-client-4-api

22e90af

We can let someone else manage Apache HttpComponents for us :)

Remove custom groupId

440d18b

This is inherited from the parent anyway. It also looks wrong.

Remove commons-codec

5b3071c

Appears to be unused in this project.

oleg-nenashev reviewed May 25, 2018

View reviewed changes

mezpahlan added 2 commits May 28, 2018 15:30

Merge branch 'master' into update-pom-structure

7259a53

# Conflicts: # pom.xml

Update Wiki URL

cd32cfe

mezpahlan self-assigned this May 28, 2018

mezpahlan merged commit 3c056df into jenkinsci:master May 28, 2018

mezpahlan deleted the update-pom-structure branch May 28, 2018 17:45

		</developers>


		<url>http://wiki.jenkins-ci.org/display/JENKINS/HockeyApp+Plugin</url>

Update pom structure #48

Update pom structure #48

Uh oh!

Conversation

mezpahlan commented Apr 15, 2018

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mezpahlan commented Apr 15, 2018

Uh oh!

oleg-nenashev commented Apr 16, 2018

Uh oh!

mezpahlan commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants