activate `failOnDryRunResults` by default

[Pankraz76]

What problem are you trying to solve?

currently its disabled by default which makes sense for some users that want to display it once.

As this tool is ment to be automated this seems to be the higher priority.

Why should i ever run dryRun (in cloud or locally), assuming i dont care for the random impl. details, just wanting to check (dryRun) and the fix (run), not wasting time one anything else, as its about automation, without failing the build?

So all the usecases i ever faced integrating rewrite into potentially maven and junit, the switch was always active. Making the current default value the exception, imposing the assumed exception is the new default.

failOnDryRunResults = true

• (re)activate rewrite capability junit-team/junit-framework#4708
• activate failOnDryRunResults by default rewrite-maven-plugin#1005
• https://github.com/openrewrite/rewrite-maven-plugin/blob/ef78a692241fdc5fb46b7131e7987ee5934502be/src/main/java/org/openrewrite/maven/AbstractRewriteDryRunMojo.java#L50

Describe the solution you'd like

Have you considered any alternatives or workarounds?

Additional context

Are you interested in contributing this feature to OpenRewrite?

[timtebeek]

Hi! Thanks for the draft PRs and starting this discussion. Let me sketch two use cases that work using dryRun without failing the build:

1. Create a .patch file for consumption by other tools, like googleapis/code-suggester or reviewdog, which post comments on PRs
2. Create data tables from recipe runs, without an interest in any code changes produced, for aggregation/processing downstream

Current integrations that rely on not failing the build would break if we now change the default. That's why I'm hesitant for now.

[Pankraz76]

1. Create a .patch file for consumption by other tools, like googleapis/code-suggester or reviewdog, which post comments on PRs

expected this, too.

integrations that rely on

yes, then simply configure there. As we change, might even fix, the CoC, they seem to be the maverick, so they should contain the config burden, once, not all the clients.

[Pankraz76]

• Create a .patch file for consumption by other tools, like googleapis/code-suggester or reviewdog, which post comments on PRs
• Create data tables from recipe runs, without an interest in any code changes produced, for aggregation/processing downstream

both cases irrelevant for junit/maven integration.

They wanted to fail early with dryRun, having simple and local approach. Assuming this is the happy path and real deal for simple customers.

I dont want to prevent the others, they seem important, but might not be the most common approach.

Which makes them the configuration-approach, and the "default" behavior, failing dryRun out of the box, is then the new convention-approach.

[timtebeek]

Ok, so then instead of changing the default now & here, let's just pass in failOnDryRunResults for the use case you're after, to avoid surprising folks who have come to rely on the current default. Thanks for the suggestion though!

[Pankraz76]

yes i think a dedicated goal named failOnDryRunResults would be better and quite self explaining.

This reduces the config burden and dryRun is like it should be intended right. Making it failing like suggestion feels off.

What about this idea?

• https://github.com/junit-team/junit-framework/pull/4708/files#r2190872906

[timtebeek]

For now I'd lean towards keeping rewriteDryRun with the option failOnDryRunResults = true set explicitly where desired. We have a pretty high bar to having two ways to do one thing, as we have such long deprecation cycles in an OSS project.

[Pankraz76]

Yes we can do it later on. Thanks.