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

Apply Checkstyle, SpotBugs, Jacoco plugins #220

Merged
merged 4 commits into from
Mar 6, 2023

Conversation

alextu
Copy link

@alextu alextu commented Feb 15, 2023

This fixes #217

Apply Checkstyle, SpotBugs, Jacoco plugins with some sensitive defaults:

  • related tasks are disabled by default, user should explicitly opt in with
jenkinsPlugin {
    spotBugsEnabled = true
    checkstyleEnabled = true
    jacocoEnabled = true
}
  • xml reports are generated

Note that SpotBugs is only compatible with Gradle 7 and above, running with Gradle 6 will just not apply the SpotBugs plugin.

  • 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 demonstrates feature works or fixes the issue

@alextu alextu marked this pull request as ready for review February 15, 2023 13:25
Copy link

@sghill sghill left a comment

Choose a reason for hiding this comment

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

Thanks @alextu! This looks great, just had a couple questions.

html.required = false
}
}
project.afterEvaluate {
Copy link

Choose a reason for hiding this comment

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

Is there a way to do this without afterEvaluate? I think the task could have an onlyIf that lazily evaluates the opt-in property?

Copy link
Author

Choose a reason for hiding this comment

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

Very good suggestion, I've made the necessary changes

@@ -92,6 +95,9 @@ class JpiExtension implements JpiExtensionBridge {
this.generateTests = project.objects.property(Boolean).convention(false)
this.requireEscapeByDefaultInJelly = project.objects.property(Boolean).convention(true)
this.generatedTestClassName = project.objects.property(String).convention('InjectedTest')
this.checkstyleEnabled = project.objects.property(Boolean).convention(false)
this.jacocoEnabled = project.objects.property(Boolean).convention(false)
this.spotBugsEnabled = project.objects.property(Boolean).convention(false)
Copy link

Choose a reason for hiding this comment

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

Do you think it makes sense to have a higher level property to allow opting into all the OSS recommendations? It's really nice to have this level of granularity, but I'm wondering if folks will basically want two modes most of the time - one for internal plugins, and one for oss plugins.

Copy link
Author

@alextu alextu Mar 6, 2023

Choose a reason for hiding this comment

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

That's a good question, how about having one single flag then configureQualityChecks (false by default) that OSS plugins devs would set to true. Internal plugin devs would apply and configure quality plugins "manually" as for any Gradle projects.
The only limitation I see is that it would require conditionnal applying plugins in afterEvaluate. That will not play well with configureQualityChecks = true and customizing a plugin with an extension, since the plugin is not applied yet when Gradle evaluates the extension. In that case the user would have to fallback to configureQualityChecks = false and configure manually as for internal plugins.
What do you think ?

Copy link

Choose a reason for hiding this comment

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

That's a really good point. Maybe it eventually makes sense to split into two plugins (oss, internal) for the different use-cases, but this is a good starting point and we can see where it evolves to. Happy to move forward with this.

@sghill sghill merged commit a6a6916 into jenkinsci:master Mar 6, 2023
@sghill
Copy link

sghill commented Mar 10, 2023

Hey @alextu wanted to get your thoughts on strategies to avoid Gradle warnings when auto-applying community plugins. I may be a bit of an outlier here, but I like to run with org.gradle.warning.mode=fail to ensure I'm not letting warnings go by unnoticed.

One downside of always applying the spotbugs plugin is we end up getting warnings from Gradle, even when it's disabled. I don't think this applies with jacoco and checkstyle because they're included by default and I've never seen a default plugin warn.

In this case the SpotBugsTask warns because it uses the deprecated ClosureBackedAction:

The org.gradle.util.ClosureBackedAction type has been deprecated. This is scheduled to be removed in Gradle 9.0. Consult the upgrading guide for further information: https://docs.gradle.org/8.0.2/userguide/upgrading_version_7.html#org_gradle_util_reports_deprecations
	at org.gradle.util.ClosureBackedAction.<clinit>(ClosureBackedAction.java:41)
...
	at com.github.spotbugs.snom.SpotBugsTask.reports(SpotBugsTask.groovy:448)
	at com.github.spotbugs.snom.SpotBugsTask$reports.call(Unknown Source)
	at org.jenkinsci.gradle.plugins.jpi.JpiPlugin$_configureSpotbugs_closure38.doCall(JpiPlugin.groovy:660)

@alextu
Copy link
Author

alextu commented Mar 13, 2023

Yes that's indeed problematic to apply plugins even if not used.
I think we could tackle this in various ways:

  1. apply them conditionnaly (see previous comment) => not really fond of using once more afterEvaluate
  2. instead of boolean flag, use functions => I implemented this quickly master...alextu:gradle-jpi-plugin:atual/fix-checks and is my prefered choice
  3. react to existing plugins, for example project.pluginManager.withPlugin('checkstyle') { // configure checkstyle => without any additional flags, it could be a surprising behavior or even mislead users regarding default behavior of Checkstyle, SpotBugs, Jacoco

What do you think ?

@alextu alextu deleted the atual/quality-plugins branch March 13, 2023 16:25
@sghill
Copy link

sghill commented Mar 17, 2023

Let's go with option 2, your preference looks good to me!

@alextu
Copy link
Author

alextu commented Mar 20, 2023

Great, here's the PR #222

@sghill
Copy link

sghill commented Apr 12, 2023

0.48.0 was released today with this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static Analysis and Coverage Reports for ci.jenkins.io
2 participants