openrewrite / rewrite-maven-plugin

OpenRewrite's Maven plugin.
https://openrewrite.github.io/rewrite-maven-plugin/plugin-info.html
Apache License 2.0
130 stars 68 forks source link

Add option to revert changes that break the build #337

Closed yeikel closed 2 years ago

yeikel commented 2 years ago

I understand that in some situations rewrite is unable to make a transformation and leaves it up to the user to continue manually. The problem that I am trying to cover with this issue is when rewrite fails unexpectedly such as when there is a bug in a recipe.

I think that we should have an option to check with the compiler before considering a transformation as successful and to revert the changes if it can't. We should also print a warning from the plugin

In general, I do not consider it acceptable to break the build when the intention is to "do not harm"

In the current workflow, we end up in situations like https://github.com/openrewrite/rewrite/issues/1698 https://github.com/openrewrite/rewrite/issues/1536 https://github.com/openrewrite/rewrite/issues/1478 that are quite unpleasant and generally a blocker until a release is made.

jkschneider commented 2 years ago

@yeikel This is probably very difficult to do generally in practice. I think our preference would be to simply fix the recipes to not make breaking changes. This is always a bug when this happens. As frustrating as that is now, is the complexity of this worth deferring those fixes to implement?

Example Scenario: Don't do anything to any source file if any one source file doesn't compile as a result

For Gradle, you could easily assemble the tasks to do this actually. For Gradle, suppose you created a grgit task rewriteRevert that does git reset --hard. You could wire rewriteRun to be always followed by a compilation and that in turn to be followed by rewriteRevert when compilation fails.

yeikel commented 2 years ago

@jkschneider Do you have any suggestion about how to do this with maven?

In my current workflow, I am running the recipes with each build both locally and in the CI environment. In a CI environment it'll be easy to do the workaround that you mentioned

As a side note, at least for cleanup recipes, I'd like to have at least a partial cleanup(and revert the impacted files) rather to do a full reset

jkschneider commented 2 years ago

As a side note, at least for cleanup recipes, I'd like to have at least a partial cleanup(and revert the impacted files) rather to do a full reset

With such a wide horizon of possible things to build, limiting the variational complexity of how the tool is used is an important part of making continuing progress and not getting mired in too many paths. I'm not sure this is clear from the repository itself or its documentation, but we were imagining that the build tool plugins always require a person to run the automation and examine results. We have not been creating a bot, preferring to instead plug in "verified by" tasks post recipe run into Moderne platform later.

That wouldn't preclude you from building a more bot-like system that post-compiles and reverts the files failing compilation. There are probably very limited applications partial rollback would be expected to work though (as you've indicated). You can also imagine how some folks wouldn't be satisfied with compilation but instead would want to run unit tests, integration tests, etc. And this is where the variational complexity could get extreme.

jkschneider commented 2 years ago

For Gradle we don't really need to add the functionality to the plugin, because it is already inherent to Gradle. You can attach a finalizedBy to any Gradle task to postprocess.

For Maven, the best you can do is add a custom Mojo that has an @Execute that runs after PROCESS_TEST_CLASSES:

image

You cannot sort mojos more directly other than to place them in different lifecycle phases.

yeikel commented 2 years ago

As a side note, at least for cleanup recipes, I'd like to have at least a partial cleanup(and revert the impacted files) rather to do a full reset

With such a wide horizon of possible things to build, limiting the variational complexity of how the tool is used is an important part of making continuing progress and not getting mired in too many paths. I'm not sure this is clear from the repository itself or its documentation, but we were imagining that the build tool plugins always require a person to run the automation and examine results. We have not been creating a bot, preferring to instead plug in "verified by" tasks post recipe run into Moderne platform later.

That wouldn't preclude you from building a more bot-like system that post-compiles and reverts the files failing compilation. There are probably very limited applications partial rollback would be expected to work though (as you've indicated). You can also imagine how some folks wouldn't be satisfied with compilation but instead would want to run unit tests, integration tests, etc. And this is where the variational complexity could get extreme.

I understand, I am clearly looking it at from a different perspective and with a different goal

I was hoping to use rewrite as a review partner similar to what Sonar is for us today

okundzich commented 2 years ago

Sonar doesn't auto remediate, it just breaks the build. You can use openrewrite in a similar way by creating a CI gate (break the build if a recipe makes any changes). but you can make it better by suggesting a remediation to developers in addition to breaking the build.

As an example see our terraform cloud integration. You can do something similar with CI and openrewrite, just would have to build a custom workflow. If the build is broken, developers can review and accept recipe result rather than having to create a fix manually. https://docs.moderne.io/how-to/terraform-cloud-integration

We treat all recipe errors as bugs, and as recipes mature hopefully there is less of them as well. So thank you for reporting issues and help us fix these bugs.