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.

Sign up for GitHub

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

Jump to bottom

WICKET-7029 Add wicket-migration module #556

Merged
merged 8 commits into from
Mar 23, 2023
Merged

WICKET-7029 Add wicket-migration module #556

merged 8 commits into from
Mar 23, 2023

Conversation

timtebeek
Copy link
Contributor

Makes it easier for users to adopt Apache Wicket 10. https://issues.apache.org/jira/browse/WICKET-7029

Migration then comes down to executing a single command:

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.apache.wicket:wicket-migration:LATEST \
  -DactiveRecipes=org.apache.wicket.BestPractices

timtebeek
timtebeek commented Mar 12, 2023
View reviewed changes
wicket-migration/src/test/java/org/apache/wicket/migration/MigrateToWicket10Test.java Outdated
java("""
package org.apache.wicket.http2.markup.head;

public class PushHeaderItem { }"""),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a quick stub to ensure the replacement below can access the type without dependency on wicket-http2-core:9.x

@timtebeek
Copy link
Contributor Author

Couple comments:

  • Wasn't sure if you'd already want to update the documentation at this stage, so the upgrade command is only listed above for now.
  • The current migration steps could all be achieved with Yaml recipes as far as I could tell; hence no custom recipe implementations yet.
  • The BestPractices recipe currently chains MigrateToWicket10, which seems sensible.
    • Alternatively you can also already add the BestPractices recipe only to the 9.x branch, and preemptively solve any deprecations there already.
  • I'll have limited time to continue work on this, but hope it's a useful start for Apache Wicket to continue.

martin-g
martin-g reviewed Mar 13, 2023
View reviewed changes
@timtebeek
Copy link
Contributor Author

There's still some dependency convergence errors; hoping the next OpenRewrite release clears at least a few of those up. Any others can then be fixed through exclusions or dependency management, unless you have other ideas there.

Apart from that I don't think there's much holding up this PR from getting merged and maintained with new breaking changes going forward.

@timtebeek timtebeek mentioned this pull request Mar 13, 2023
Closed
@timtebeek
Copy link
Contributor Author

So the bump of the rewrite-recipe-bom helped; now there's only one enforcer rule violation left:

Warning:  
Dependency convergence error for jakarta.activation:jakarta.activation-api:jar:1.2.2:runtime paths to dependency are:
+-org.apache.wicket:wicket-migration:jar:10.0.0-M1-SNAPSHOT
  +-org.openrewrite.recipe:rewrite-migrate-java:jar:1.18.0:compile
    +-org.openrewrite:rewrite-maven:jar:7.38.0:runtime
      +-com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.13.4:runtime
        +-jakarta.xml.bind:jakarta.xml.bind-api:jar:2.3.3:runtime
          +-jakarta.activation:jakarta.activation-api:jar:1.2.2:runtime
and
+-org.apache.wicket:wicket-migration:jar:10.0.0-M1-SNAPSHOT
  +-org.openrewrite.recipe:rewrite-migrate-java:jar:1.18.0:compile
    +-org.openrewrite:rewrite-maven:jar:7.38.0:runtime
      +-com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.13.4:runtime
        +-jakarta.activation:jakarta.activation-api:jar:1.2.1:runtime

Error:  Rule 1: org.apache.maven.plugins.enforcer.DependencyConvergence failed with message:
Failed while enforcing releasability. See above detailed error message.

Which version do you suggest we pick & how should we enforce this; dependencyManagement for jakarta.activation:jakarta.activation-api:jar:1.2.2:runtime ?

@martin-g
Copy link
Member

Please rebase to latest master. There was a commit earlier today that updated the dependencies.

timtebeek and others added 4 commits March 15, 2023 12:30
@timtebeek
WICKET-7029 Add wicket-migration module
f7cf1b6 
Makes it easier for users to adopt Apache Wicket 10.
https://issues.apache.org/jira/browse/WICKET-7029

Migration then comes down to executing a single command:
```shell
mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.apache.wicket:wicket-migration:LATEST \
  -DactiveRecipes=org.apache.wicket.BestPractices
```
@timtebeek
Delete Main.java
57ec06a
@timtebeek
Drop wicket-core dependency as it's not needed for current migrations…
8abe60c 
… or tests
@timtebeek
rewrite-recipe-bom 1.17.0
e5c5408
@timtebeek timtebeek force-pushed the WICKET-7029_wicket-migration branch from c603f19 to e5c5408 Compare March 15, 2023 11:31
@timtebeek timtebeek marked this pull request as ready for review March 18, 2023 20:13
@timtebeek timtebeek requested a review from martin-g March 21, 2023 12:09
martin-g
martin-g approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

LGTM!

@martin-g martin-g merged commit 16e6376 into apache:master Mar 23, 2023
@martin-g
Copy link
Member

Big thank you, @timtebeek !

@timtebeek timtebeek deleted the WICKET-7029_wicket-migration branch March 23, 2023 11:37
@timtebeek
Copy link
Contributor Author

Awesome, great to see this made it in! Happy to discuss whenever you need help for further migration steps. :)

@martin-g
Copy link
Member

Added an item to the Migration Page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-g martin-g martin-g approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timtebeek @martin-g