Skip to content
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

Enable Warnings Next Generation #5752

Merged
merged 4 commits into from
Oct 5, 2021
Merged

Conversation

basil
Copy link
Member

@basil basil commented Sep 27, 2021

Fixes Javadoc warnings, then enables the Warnings Next Generation plugin as in jenkins-infra/pipeline-library#121. I tested this both with and without Checkstyle/SpotBugs/Javadoc violations locally and in CI.

@basil basil force-pushed the warnings-ng branch 4 times, most recently from e6c3acc to 97ba6e1 Compare September 27, 2021 05:53
@basil basil added the skip-changelog Should not be shown in the changelog label Sep 27, 2021
@basil basil marked this pull request as ready for review September 27, 2021 06:21
sourceCodeEncoding: 'UTF-8',
skipBlames: true,
trendChartType: 'TOOLS_ONLY',
qualityGates: [[threshold: 1, type: 'NEW', unstable: true]]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that we could also enable spotbugs on medium and make it a lot harder that new issues get introduced.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but how do you reproduce locally is it a matter of enabling medium and searching?

Not a huge fan of the approach but probably better to do so that new issues don’t creep in till we change the level

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of the approach

I think you're seeing why I didn't go that far in this PR. :-) Yes, that is a more controversial change. I'm not a fan of using the CI system as a ratchet either: I'd rather use the build system for that. But I don't feel strongly either way. In any case that shouldn't matter for this PR, which preserves the status quo.

@@ -425,7 +425,6 @@ THE SOFTWARE.
<!-- Version specified in parent POM -->
<configuration>
<consoleOutput>true</consoleOutput>
<failsOnError>true</failsOnError>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the Checkstyle plugin, right? Would be nice to retain this behavior for non-CI builds, e.g. via a default profile

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the Checkstyle plugin, right?

Yes.

Would be nice to retain this behavior for non-CI builds

The existing behavior is retained for non-CI builds via <failOnViolation>, which defaults to true. <failsOnError> is unnecessary to retain the existing behavior.

@timja
Copy link
Member

timja commented Sep 27, 2021

@basil
Copy link
Member Author

basil commented Oct 2, 2021

Is it right that spotbugs reports no issues?

Yes, it is. This is because we have <spotbugs.threshold>High</spotbugs.threshold> in core/pom.xml. At this threshold, there are no SpotBugs issues.

@basil basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Oct 3, 2021
@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 4, 2021
@oleg-nenashev
Copy link
Member

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@oleg-nenashev oleg-nenashev merged commit 3d6d8ff into jenkinsci:master Oct 5, 2021
@basil basil deleted the warnings-ng branch October 5, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants