Run OpenRewriteRecipeBestPractices on pull requests

[timtebeek]

As discussed on

• Pull #87: Apply OpenRewrite recipe best practices #87 (comment)

The goal here is to run recipes, report violations with a suggested code fix, and in that way keep new issues out, as documented:

• https://www.moderne.io/blog/stop-breaking-ci-annotate-prs-with-openrewrite-recipe-fixes-as-quality-gate

[timtebeek]

The comment-pr workflow runs from the main branch; so this PR needs to be merged before seeing the full effect.

[romani]

Error: Failed to execute goal org.openrewrite.maven:rewrite-maven-plugin:6.16.0:run (default-cli) on project checkstyle-openrewrite-recipes: Execution default-cli of goal org.openrewrite.maven:rewrite-maven-plugin:6.16.0:run failed: Failed to parse checkstyle XML report from: target/checkstyle/checkstyle-report.xml: target/checkstyle/checkstyle-report.xml (No such file or directory) -> [Help 1]

Is this a concern ?

[timtebeek]

I think that might be the self-check that's configured on this project; how intentional were you in running those checks here too?

checkstyle-openrewrite-recipes/pom.xml

Lines 174 to 177 in 00ff3ba

	<configuration>
	<activeRecipes>
	<recipe>CheckstyleAutoFixConfigured</recipe>
	</activeRecipes>

1. We can merge the recipes to run on this project, and then would also need to invoke checkstyle to create the xml
2. I can move the recipe I want to run into a Maven profile and activate without a need for checkstyle xml

Any preference between those options?

[rdiachenko]

how intentional were you in running those checks here too?

It was intentional. We want to run all recipes we create against ourselves. I think we should go with option 1, run checkstyle before openrewrite, and run auto-fix and best practices recipes afterwards

[timtebeek]

Ok I can't seem to get it to produce the report you're after locally; any help appreciated!

[rdiachenko]

Ok I can't seem to get it to produce the report you're after locally; any help appreciated!

Looks like we need to move <configuration> block to a global level within the checktyle plugin:

            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-checkstyle-plugin</artifactId>
                <version>${maven.checkstyle.plugin.version}</version>
                <dependencies>
                ...
                </dependencies>

<!-- This configuration block is currently under <execution> and needs to be moved to this level -->
                <configuration>
                ...
                </configuration>

                <executions>
                    <execution>
                        <id>check</id>
                        <goals>
                            <goal>check</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>

At the moment it is on the <execution> level and works during check maven goal only. When you run mvn checkstyle:checkstyle -Dcheckstyle.failOnViolation=false it doesn't see needed properties and you need to set them manually like this:

mvn checkstyle:checkstyle \
  -Dcheckstyle.config.location=https://raw.githubusercontent.com/checkstyle/checkstyle/checkstyle-10.26.1/config/checkstyle-checks.xml \
  -Dcheckstyle.output.file=target/checkstyle/checkstyle-report.xml \
  -Dcheckstyle.output.format=xml \
  -Dcheckstyle.properties.location=config/checkstyle.properties \
  -Dcheckstyle.failOnViolation=false

By making configuration global should allow to avoid setting those properties manually.

[rdiachenko]

lgtm

@timtebeek could you squash all commits into one please and prefix it with PR number please?

[romani]

not sure if we need to change commit message in such updates, but done.

@timtebeek , please finalize review

[romani]

ok to merge and play with it.

[romani]

@timtebeek , do you have visual examples in web on how this workflow works?

[timtebeek]

Yes sure! The blog above links to this pull request that has a few of such resolved comments

• Add Transient annotation to Collections without JPA annotations openrewrite/rewrite-migrate-java#421

[romani]

this approach if very noisy, we tried it with Sonar, long time ago, it produce too much comments, and PRs started to load slow and we need to close PR and open it again. When you have pre-junior contributors as majority in projects, this is not very good approach. Just failed CI and contributors open failed job to see suggestions is better.

in slow traffic project and experienced contributors it will work.

we can try it in this project and see how it will go.

[timtebeek]

Yes I find it works best when the suggestions are immediately applied, but I do like how contextualized they are and that they don't break flow ideally: just press a button and move on. I hope that'll be your experience as well.

[timtebeek]

not sure if we need to change commit message in such updates, but done.

@timtebeek , please finalize review

Thanks for squashing those commits! Looks good on my end.

Out of curiosity: any reason you all aren't using the squash option in GitHub?
We use that extensively, but I'm wondering if I've missed any nuance there.

[romani]

Out of curiosity: any reason you all aren't using the squash option in GitHub?

in main repo, commits generate release notes, so it critical to follow format and some format.
second reason we keep git history clear for next engineers, nobody like to deal with project that has mess, we(all others) are not payed to deal with project that is messy (no incentive to tolerate mess). So rule of thumb, each contrbutor, future and past, in same position - no mess of any kind.
It helps dramatically in long term support of project when you have 15 minutes per day to help with keep project running.

In this project, effect is not seen yet, but if you open main library project, you can trace all changes and all decisions and you can detangle even very nuanced issues: who , when, why.
Some time we do few commits in PR, when each commit is fully independent update.