Skip to content

document the various code quality tools in our Developer FAQ#1038

Merged
epugh merged 4 commits intoapache:mainfrom
epugh:document_code_quality
Sep 28, 2022
Merged

document the various code quality tools in our Developer FAQ#1038
epugh merged 4 commits intoapache:mainfrom
epugh:document_code_quality

Conversation

@epugh
Copy link
Contributor

@epugh epugh commented Sep 22, 2022

https://issues.apache.org/jira/browse/SOLR-XXXXX

Description

there has been some great work around code quality recently, and I realized that I don't really know what all the tools are that are available... So what is a new person going to do?

Solution

Add an entry to the developer docs FAQ.

Comment on lines 81 to 83
We also check for security vulnerable components in Solr using the
[OWASP](https://plugins.gradle.org/plugin/org.owasp.dependencycheck) Gradle plugin, as part of the
`check` task.
Copy link
Contributor

Choose a reason for hiding this comment

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

OWASP doesn't run by default. ./gradlew check -x test -Dvalidation.owasp=true with the parameter is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can also be run independently with ./gradlew owasp.
There was a plan to run OWASP checks in Jenkins, but I cannot find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworked the text

@epugh
Copy link
Contributor Author

epugh commented Sep 23, 2022

Do we think it's useful to add the RAT and other validations??? Or are we going beyond FAQ... I was on the fence about OWASP...

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I think we should add Sonatype Lift to this list.

Link: https://lift.sonatype.com/

Lift is an automated code reviewer. As with human reviewers, it's a good practice to respond to each comment to reflect on the recommendation. Of course, automated code analysis will turn up false-positives. Tip: There is no point in responding to Lift with the ignore command because we don't gate PRs on Lift.


=== How do we ensure coding standards and quality?

We use a number of tools for ensuring that Solr's codebase follows our community standards. The most
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: when writing adoc files, consider one sentence per line. It helps with diffs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I guess we don't have a "we want 80 charaters more or less per line"? We don't have lint on that today... I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't mandatory but is good-practice. Cassandra did lots of such changes over the ref guide, and I'm grateful for it.

[Error Prone](https://errorprone.info/) goes beyond static type checking and recommends fixes
for common bug patterns. Error Prone is normally run during a CI build, to run it locally via:

`./gradlew check -Pvalidation.errorprone=true`
Copy link
Contributor

Choose a reason for hiding this comment

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

that will also run tests; right? instead run the specific task if that's what we're trying to show someone.

Copy link
Contributor

Choose a reason for hiding this comment

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

In your latest commit, you improved this to exclude tests so it's "good enough" but nonetheless, it'd be nice to show someone how to directly run this specific validation (as we do above for OWASP) rather than an entire build phase (check) and exclude various things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to find a actual "gradle target" (I think the word is) and didn't have any luck. I think it's welded directly into the check behavior...

Copy link
Contributor

Choose a reason for hiding this comment

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

I turned up nothing as well; seems hooked into the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I am going to pretend to be @gerlowskija and say "Teperating this into it's own target is probably a seperate PR"...

So, do you think this is ready to commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely; I approved 6 hours ago.
Thanks!

@sonatype-lift
Copy link

sonatype-lift bot commented Sep 27, 2022

⚠️ 313 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@epugh epugh merged commit ff73337 into apache:main Sep 28, 2022
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.

4 participants