feature : support warnings-ng plugin [JENKINS-57427] #221

cyrille-leclerc · 2019-06-06T08:03:57Z

support warnings-ng plugin, and remove deprecated findbugs and tasks plugins support

See JENKINS-57427

Testing done

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests that demonstrate the feature works or the issue is fixed

jenkins-plugin/pom.xml

...lugin/src/main/java/org/jenkinsci/plugins/pipeline/maven/publishers/WarningsNgPublisher.java

jglick

This should also delete the findbugs plugin dep.

CC @bguerin @uhafner

bguerin · 2022-09-12T21:58:38Z

This should also delete the findbugs plugin dep.

Hello @jglick Yes I know. I also really want to do this integration and to remove deprecated dependencies. It is on my top level TODO list. For now, I only do bug fixing to discover the plugin internals, and next then I will work on this

...e-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/publishers/WarningsPublisher.java

…roblems

bguerin · 2025-08-28T08:20:12Z

@jglick @olamy (and others) I would love a review if you have some time. Thanks !

jglick · 2025-09-02T22:22:32Z

Sorry, I do not have much capacity for reviews.

olamy · 2025-09-03T00:01:32Z

pipeline-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/WithMavenStep.java

    private String globalMavenSettingsFilePath = "";
    private String maven;
-    private String mavenOpts = "";
+    private String mavenOpts =


do I understand correctly every build will now use this per default?
looks like some breaking changes. If used per default this will override pom properties from users

Yeah. I did that because without, the maven build will fail at first problem, the integration won't run and you will not have the reports in Jenkins, and the build as unstable but failed without knowing why, only the logs will help

But you are right, it is too intrusive, it is usefull at most if you rely on automatic integrations, will change that.

and good catch for pom properties, will take care of that too

Thanks !

olamy · 2025-09-03T00:27:17Z

@bguerin no much time for a very detailed review. I added a question about what looks to be the new default mavenOpts value, which seems to be a bit intrusive to me, especially the -Dmaven.test.failure.ignore

jtnord

the code will not work correct if the warnings plugin is not installed.

Additionally this breaks existing installations by remove support without providing a time to migrate.

jtnord · 2025-09-03T08:34:28Z

pipeline-maven-ui-tests/src/main/java/org/jenkinsci/plugins/maven/WithMaven.java

        private final Control skipDownstreamTriggers = control("skipDownstreamTriggers");
        private final Control ignoreUpstreamTriggers = control("ignoreUpstreamTriggers");

+        private Control sourceCodeEncoding = control("sourceCodeEncoding");


I would have this page should be generic for all publishers and specific publisher functionality / options should be in a sepearate page area for that Publisher. (e.g. see Credentials -> https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/main/java/org/jenkinsci/test/acceptance/plugins/credentials/StringCredentials.java)

...e-maven-ui-tests/src/test/java/org/jenkinsci/plugins/maven/GlobalSnippetGeneratorUiTest.java

jtnord · 2025-09-03T08:38:12Z

...e-maven-ui-tests/src/test/java/org/jenkinsci/plugins/maven/GlobalSnippetGeneratorUiTest.java

+
+        assertThat(snippetGenerator.generateScript())
+                .isEqualTo("""
+withMaven(options: [warningsPublisher()], traceability: true) {


this would appear incorrect.
you have used the defaults but with a specific publisher and yet traceability: true is present. if this is a default it should not have been generated by the snippet generator.

...e-maven-ui-tests/src/test/java/org/jenkinsci/plugins/maven/GlobalSnippetGeneratorUiTest.java

pipeline-maven-ui-tests/src/main/java/org/jenkinsci/plugins/maven/WithMaven.java

...urces/org/jenkinsci/plugins/pipeline/maven/publishers/FindbugsAnalysisPublisher/help_fr.html

...ain/resources/org/jenkinsci/plugins/pipeline/maven/publishers/InvokerRunsPublisher/help.html

...in/resources/org/jenkinsci/plugins/pipeline/maven/publishers/JacocoReportPublisher/help.html

jtnord · 2025-09-03T10:08:20Z

...resources/org/jenkinsci/plugins/pipeline/maven/publishers/TasksScannerPublisher/config.jelly


    <st:include page="maven-publisher" class="${descriptor.clazz}"/>

-    <f:section title="${%cource_code}">


please don;t break existing setups for no reason without time to migrate.

uhafner · 2025-09-03T11:24:13Z

I have some general concerns with this PR.

One the one hand, it would make more sense to remove all those deprecated publisher totally (maybe in a separate PR). It makes no sense to report some logger warnings that users need to update now. Those steps are deprecated for years now, and most of the referenced plugins even do not exist anymore. It would simplify your development when you delete this code.

One the other hand, I am not convinced that the overall approach makes sense. You are remodeling the options of the warnings and coverage plugins. This seems to be redundant. And very fragile when one of those plugins change. I wonder why you do not simply publish the corresponding reports with the default values without any user configuration (just enable/disable). And if users need additional options, they still can use the correct warnings and coverage steps of the corresponding plugins.

BTW: you are also using a lot of code of the warnings and coverage plugins that are not meant to be used as API. This might break in the future as well...

jtnord · 2025-09-03T16:42:08Z

One the other hand, I am not convinced that the overall approach makes sense. You are remodeling the options of the warnings and coverage plugins. This seems to be redundant. And very fragile when one of those plugins change. I wonder why you do not simply publish the corresponding reports with the default values without any user configuration (just enable/disable). And if users need additional options, they still can use the correct warnings and coverage steps of the corresponding plugins.

What I like about not having to configure the (old publishers) is that the configuration was correctly obtained and specific to a build without human intervention or mundane maintainance.

For example if is have a multi-project maven setup but have some modules that are not part of the build I would not want any of their TODOs to show up. Obtaining the source paths from maven means this is handled and I can then reuse common pipelines across many different projects. (e.g. **/src/main/java would pick up things that are not part of the build etc). The new code does not appear to do this which makes it less useful for me.

If I have to use warnings-ng to do this then I can not have a generic pipeline for my jobs. So there is a use case here - and I think it is an important one.

As for using the internals of warnings-ng that could change and break, perhaps the integration should move the warnings-ng plugin so that any changes/refactoring can happen in a single repository (although that also has some downsides for split maintenance) and that would be something that Ulli would need to be OK with.

bguerin · 2025-09-03T18:55:29Z

Many many thanks @jtnord for your review, will handle all your points

the code will not work correct if the warnings plugin is not installed.

Why ? It should by the way ...

Additionally this breaks existing installations by remove support without providing a time to migrate.

Agree, I saw https://issues.jenkins.io/browse/JENKINS-76003 and https://issues.jenkins.io/browse/JENKINS-76032

I think about reverting to @Extension instead of OptionalExtension to avoid some problems. Is that enough (I saw your comment about jelly configs) ?

bguerin · 2025-09-03T18:59:10Z

One the other hand, I am not convinced that the overall approach makes sense. You are remodeling the options of the warnings and coverage plugins. This seems to be redundant. And very fragile when one of those plugins change. I wonder why you do not simply publish the corresponding reports with the default values without any user configuration (just enable/disable). And if users need additional options, they still can use the correct warnings and coverage steps of the corresponding plugins.

What I like about not having to configure the (old publishers) is that the configuration was correctly obtained and specific to a build without human intervention or mundane maintainance.

For example if is have a multi-project maven setup but have some modules that are not part of the build I would not want any of their TODOs to show up. Obtaining the source paths from maven means this is handled and I can then reuse common pipelines across many different projects. (e.g. **/src/main/java would pick up things that are not part of the build etc). The new code does not appear to do this which makes it less useful for me.

If I have to use warnings-ng to do this then I can not have a generic pipeline for my jobs. So there is a use case here - and I think it is an important one.

I have the same opinion than James, I am trying to follow Maven philosophy : convention about configuration.
I put some configuration to avoid hard coded values in the code, and tries to have the less of them. If you think it is too much, maybe some can be removed.

As for using the internals of warnings-ng that could change and break, perhaps the integration should move the warnings-ng plugin so that any changes/refactoring can happen in a single repository (although that also has some downsides for split maintenance) and that would be something that Ulli would need to be OK with.

I think this is not the job of warnings-ng to have this integration. It would need warnings-ng to know Maven world ... this is the job of this plugin.
I know the code use warnings-ng internal and it could break anytime, maybe it miss more unit tests, and more robustness to ensure compatibility even if warnings-ng evolve somehow.

jtnord · 2025-09-04T09:48:01Z

As for using the internals of warnings-ng that could change and break, perhaps the integration should move the warnings-ng plugin so that any changes/refactoring can happen in a single repository (although that also has some downsides for split maintenance) and that would be something that Ulli would need to be OK with.

I think this is not the job of warnings-ng to have this integration. It would need warnings-ng to know Maven world ... this is the job of this plugin. I know the code use warnings-ng internal and it could break anytime, maybe it miss more unit tests, and more robustness to ensure compatibility even if warnings-ng evolve somehow.

Does not have to be the warninings-ng plugin but another plugin in the same repo that adds the support. (aka a bridge plugin).

jtnord · 2025-09-04T09:51:25Z

Many many thanks @jtnord for your review, will handle all your points

the code will not work correct if the warnings plugin is not installed.

Why ? It should by the way ...

for clarity -> #221 (comment)

jglick reviewed Mar 30, 2020

View reviewed changes

jenkins-plugin/pom.xml Outdated Show resolved Hide resolved

...lugin/src/main/java/org/jenkinsci/plugins/pipeline/maven/publishers/WarningsNgPublisher.java Outdated Show resolved Hide resolved

aheritier added the wip label Aug 8, 2020

aheritier marked this pull request as draft August 8, 2020 13:19

aheritier self-assigned this Aug 8, 2020

aheritier removed their assignment Aug 30, 2020

jglick reviewed Sep 12, 2022

View reviewed changes

bguerin changed the title ~~WIP JENKINS-57427 support warnings-ng plugin~~ feature : support warnings-ng plugin [JENKINS-57427] Aug 11, 2025

bguerin force-pushed the JENKINS-57427-2 branch from 0c58ff3 to 8107b01 Compare August 11, 2025 13:11

bguerin self-assigned this Aug 11, 2025

bguerin added the enhancement label Aug 11, 2025

bguerin force-pushed the JENKINS-57427-2 branch 2 times, most recently from e8c7d56 to 22de135 Compare August 20, 2025 20:30

github-advanced-security bot found potential problems Aug 20, 2025

View reviewed changes

...e-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/publishers/WarningsPublisher.java Dismissed Show dismissed Hide dismissed

...e-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/publishers/WarningsPublisher.java Dismissed Show dismissed Hide dismissed

bguerin force-pushed the JENKINS-57427-2 branch 6 times, most recently from c3fa2a1 to 7421bc5 Compare August 24, 2025 12:26

github-advanced-security bot found potential problems Aug 24, 2025

View reviewed changes

...e-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/publishers/WarningsPublisher.java Dismissed Show dismissed Hide dismissed

bguerin force-pushed the JENKINS-57427-2 branch 3 times, most recently from 33a4c2b to 1cf7485 Compare August 24, 2025 21:03

cyrille-leclerc and others added 5 commits August 25, 2025 09:06

JENKINS-57427 support warnings-ng plugin

3698cc7

modernize

6bef08a

remove old publishers

6388bfc

warningsNgPublisher => warningsPublisher

7da5870

documentation

031a4d0

bguerin force-pushed the JENKINS-57427-2 branch from 1cf7485 to 855662d Compare August 25, 2025 07:06

bguerin force-pushed the JENKINS-57427-2 branch from 855662d to ad80822 Compare August 25, 2025 18:50

bguerin added 5 commits August 26, 2025 20:30

handle findbugs and spotbugs

ccf5c15

handle maven console

657f412

handle Java and JavaDoc

73222b5

handle open tasks

23f8c94

handle PMD and CPD

daf4546

bguerin force-pushed the JENKINS-57427-2 branch from ad80822 to 6b568ab Compare August 26, 2025 18:54

bguerin added 3 commits August 27, 2025 08:51

handle checkstyle

741c831

add configuration

052c70f

tests

aa33b1c

bguerin force-pushed the JENKINS-57427-2 branch from 6b568ab to aa33b1c Compare August 27, 2025 06:54

Mark deprecated publishers as OptionalExtension

16b3f97

bguerin force-pushed the JENKINS-57427-2 branch from 3166ac9 to 16b3f97 Compare August 28, 2025 07:10

Add default maven options to let build finish even with tests/style p…

18695f9

…roblems

bguerin marked this pull request as ready for review August 28, 2025 08:18

bguerin requested a review from a team as a code owner August 28, 2025 08:18

olamy reviewed Sep 3, 2025

View reviewed changes

jtnord suggested changes Sep 3, 2025

View reviewed changes


		<st:include page="maven-publisher" class="${descriptor.clazz}"/>

		<f:section title="${%cource_code}">

feature : support warnings-ng plugin [JENKINS-57427] #221

Are you sure you want to change the base?

feature : support warnings-ng plugin [JENKINS-57427] #221

Uh oh!

Conversation

cyrille-leclerc commented Jun 6, 2019 • edited by bguerin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing done

Submitter checklist

Uh oh!

Uh oh!

Uh oh!

jglick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bguerin commented Sep 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bguerin commented Aug 28, 2025

Uh oh!

jglick commented Sep 2, 2025

Uh oh!

olamy Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bguerin Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

olamy commented Sep 3, 2025

Uh oh!

jtnord left a comment

Choose a reason for hiding this comment

Uh oh!

jtnord Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtnord Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtnord Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

uhafner commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtnord commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bguerin commented Sep 3, 2025

Uh oh!

bguerin commented Sep 3, 2025

Uh oh!

jtnord commented Sep 4, 2025

Uh oh!

jtnord commented Sep 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cyrille-leclerc commented Jun 6, 2019 •

edited by bguerin

Loading

jglick left a comment •

edited

Loading

olamy Sep 3, 2025 •

edited

Loading

uhafner commented Sep 3, 2025 •

edited

Loading

jtnord commented Sep 3, 2025 •

edited

Loading