-
Notifications
You must be signed in to change notification settings - Fork 798
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.
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
Add Migration module with OpenRewrite recipes for AxonFramework 4.7 #2597
Conversation
migration/src/test/java/org/axonframework/migration/AxonJakartaTest.java
Show resolved
Hide resolved
Migration guide mistakenly uses axon-config-jakarta, which does not exist.
Tests were failing before because the migration guide mistakenly mentions |
Thanks for the concern, @timtebeek. All the better that you're providing a pull request for the matter. So, thanks for doing this, @timtebeek :-) |
You're welcome; just happy to help, and with the initial module in place it's not that hard to maintain I would say. No idea how to resolve the final check failure; will you take it from here?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First and foremost, thanks for the effort you've put in here, @timtebeek!
I've got quite a bunch of comments. Some of those details of the migration, but also several around how this works.
migration/src/test/java/org/axonframework/migration/AxonJakartaTest.java
Outdated
Show resolved
Hide resolved
migration/src/test/java/org/axonframework/migration/AxonJakartaTest.java
Show resolved
Hide resolved
Quick apply during lunch. Co-authored-by: Steven van Beelen <[email protected]>
Applied the first round of suggestion and answered a couple of questions, any other comments will have to wait until after work. You're welcome to push changes too, just so you know! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concerns have been addressed. Hence I'm approving this pull request.
There's still some test work left I assume, but I don't feel like blocking this PR for that.
@gklijs, what's your take on this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and complete to me. Just not sure about using the snapshot versions in the readme.
Fixes #2596. Rough draft for now, to show you the outline of how I would approach this; edits welcome by maintainers. (@gklijs)